Skip to content

Port liquid-c bug compatible whitespace trimming#1291

Merged
dylanahsmith merged 2 commits into
masterfrom
port-liquid-c-bug-compatible-whitespace-trimming
Sep 16, 2020
Merged

Port liquid-c bug compatible whitespace trimming#1291
dylanahsmith merged 2 commits into
masterfrom
port-liquid-c-bug-compatible-whitespace-trimming

Conversation

@dylanahsmith
Copy link
Copy Markdown
Contributor

@dylanahsmith dylanahsmith commented Sep 8, 2020

Problem

As part of the work to integrate Shopify/liquid-c#58, we need to opt-out of using Liquid::C::BlockBody in some code to debug verification failures as mentioned in that PR:

As part of our storefront rewrite we have some code to debug differences in the rendered output, which is quite coupled to the liquid parse tree node ruby objects, but doesn't seem to be performance sensitive, so I added a :disable_liquid_c_nodes parse option so this can be used without disabling liquid-c globally (which wouldn't be thread-safe).

Unfortunately, using Liquid::BlockBody for parsing can result in an incompatibility with liquid-c when relying on its bug_compatible_whitespace_trimming: true parse option. This is the same incompatibility issue we were seeing into when testing TruffleRuby with liquid and not liquid-c. So I would like to port that parse option to unblock these use cases.

Solution

The bug prevents pre-trimming from removing the first character in the preceding string token. So we can just restore that byte after stripping if previous_token.rstrip! fully strips the token (i.e. previous_token was a non-empty blank string).

The tests were copied from the above mentioned liquid-c pull request, which we can now remove them from that repo after this is merged.

@dylanahsmith dylanahsmith requested review from XrXr and pushrax September 8, 2020 18:26
@dylanahsmith dylanahsmith changed the title Port liquid-c bug comaptible whitespace trimming Port liquid-c bug compatible whitespace trimming Sep 9, 2020
Copy link
Copy Markdown
Contributor

@pushrax pushrax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it makes anything easier, we're not really using the highly-coupled Liquid diff debugging system in SFR, it's actually somewhat buggy itself. It could be removed entirely.

@dylanahsmith
Copy link
Copy Markdown
Contributor Author

Ok, I'll delete that code if it causes more problems. However, I still think it would be useful to be able to dynamically use ruby liquid in a thread-safe way for verification in the future, in which case having them be compatible will be necessary. That could make it easier to detect incompatibilities, like this whitespace trimming bug, in production.

@dylanahsmith dylanahsmith force-pushed the port-liquid-c-bug-compatible-whitespace-trimming branch from 5574aad to 8a73c5b Compare September 11, 2020 13:47
@dylanahsmith dylanahsmith force-pushed the port-liquid-c-bug-compatible-whitespace-trimming branch from 3a42231 to 771b123 Compare September 11, 2020 14:30
assert_template_result(expected, text)
end

def test_pre_trim_blank_preceding_text
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a commit to port this test too, which is the corresponding test without the bug_compatible_whitespace_trimming: true option

Copy link
Copy Markdown

@XrXr XrXr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My memory on this is hazy but the change makes sense when I review it with related changes.
Related PRs:

@dylanahsmith dylanahsmith merged commit ca4b9b4 into master Sep 16, 2020
@dylanahsmith dylanahsmith deleted the port-liquid-c-bug-compatible-whitespace-trimming branch September 16, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants