Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Theme] Avoid breaking the document on Liquid syntax error #4583

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Oct 4, 2024

WHY are these changes introduced?

Fixes #4548

We were prettifying Liquid syntax errors to make them more obvious. The problem is that, in doing so, we were breaking the rest of the documents if the Liquid syntax error ocurred within CSS or an HTML attribute. This affected themes that had inadvertent Liquid errors. The Ruby CLI would just display the Liquid error within the HTML attribute, making it virtually invisble, while the new CLI would make it obvious and break the rest of the document.

We might reintroduce the Liquid error prettifyier / surface them with an overlay or something that doesn't affect the rest of the document, so that it is obvious you have Liquid errors.

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@frandiox frandiox requested a review from a team October 4, 2024 11:54
@frandiox
Copy link
Contributor Author

frandiox commented Oct 4, 2024

/snapit

Copy link
Contributor

github-actions bot commented Oct 4, 2024

🫰✨ Thanks @frandiox! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/cli@0.0.0-snapshot-20241004115504

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Copy link
Contributor

github-actions bot commented Oct 4, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.73% (+0.01% 🔼)
8563/11774
🟡 Branches
69.72% (+0.02% 🔼)
4205/6031
🟡 Functions 71.69% 2211/3084
🟡 Lines
73.06% (+0.01% 🔼)
8105/11094

Test suite run success

1947 tests passing in 876 suites.

Report generated by 🧪jest coverage report action from f953824

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @frandiox! Excellent finding! LGTM and works as expected as well 🎩

I believe we just need to solve the conflict in the branch and then we will be ready to merge :)

Thanks again for this PR!

@frandiox frandiox added this pull request to the merge queue Oct 7, 2024
Merged via the queue into main with commit e4139d0 Oct 7, 2024
36 checks passed
@frandiox frandiox deleted the fd-remove-liquid-error-prettifier branch October 7, 2024 14:36
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.

[Bug]: Site becomes broken using new CLI
3 participants