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

Provide parser error when :global appears without selector in CSS #4936

Merged
merged 2 commits into from
Jun 8, 2020
Merged

Provide parser error when :global appears without selector in CSS #4936

merged 2 commits into from
Jun 8, 2020

Conversation

CMessinides
Copy link
Contributor

Fixes #4930

Prevent a compiler crash when :global appears by itself in a CSS selector by adding an extra check when walking the style AST. If a node meets the following conditions:

  • type is 'PseudoClassSelector',
  • name is 'global', and
  • children is null,

then the parser reports an error: :global() must contain a selector, cannot be used on its own.

Long-time user, first-time contributor -- please forgive any naive errors in putting together this PR. In particular, I wasn't quite sure of the right way to report the location of the error. In the file I touched, src/compiler/parse/read/style.ts, one of the existing checks uses the node's loc.start.offset property, while the other uses the start property. The former seemed to make the most sense with the test case I wrote, so I went with that -- but happy to correct if I was wrong.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@tanhauhau
Copy link
Member

The former seemed to make the most sense with the test case I wrote,

yea the latter one does not make any sense to me too.. maybe we should fix it together? 🙈

@tanhauhau
Copy link
Member

but besides that, overall looks good to me 👍

@Conduitry
Copy link
Member

I'd like to avoid adding more compiler error codes whenever possible, because these become a weird undocumented kinda-public part of our API. Is there an existing one that makes more sense here? Maybe just the general one that we use when the underlying CSS-parsing library throws an exception?

@Conduitry Conduitry merged commit e46e1af into sveltejs:master Jun 8, 2020
@CMessinides
Copy link
Contributor Author

@Conduitry sorry for the radio silence on this -- code just wasn't front of mind last week. Thanks for making that change and happy to see it merged in.

taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
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.

Compiler crashes with orphaned :global in CSS
3 participants