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

Discard invalid declarations when parsing CSS #16093

Merged
merged 6 commits into from
Jan 31, 2025

Conversation

thecrypticace
Copy link
Contributor

I discovered this when triaging an error someone had on Tailwind Play.

  1. When we see a ; we often assume a valid declaration precedes it but that may not be the case
  2. When we see the name of a custom property we assume everything that follows will be a valid declaration but that is not necessarily the case
  3. A bare identifier inside of a rule is treated as a declaration which is not the case

This PR fixes all three of these by ignoring these invalid cases. Though some should probably be turned into errors.

@RobinMalfait RobinMalfait force-pushed the fix/discard-invalid-decls branch from a92ff26 to 5ba4703 Compare January 31, 2025 10:49
Comment on lines +332 to +353
it('should parse a custom property with an empty value', () => {
expect(parse('--foo:;')).toEqual([
{
kind: 'declaration',
property: '--foo',
value: '',
important: false,
},
])
})

it('should parse a custom property with a space value', () => {
expect(parse('--foo: ;')).toEqual([
{
kind: 'declaration',
property: '--foo',
value: '',
important: false,
},
])
})

Copy link
Member

Choose a reason for hiding this comment

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

Added these just in case we broke something with unexpected semicolon detection

})

it('should error when consecutive semicolons exist', () => {
expect(() => parse(';;;')).toThrowErrorMatchingInlineSnapshot(`[Error: Unexpected: \`;;;\`]`)
Copy link
Member

Choose a reason for hiding this comment

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

We could also just throw the same Unexpected semicolon error, I'm indifferent on this one.

I think once we have proper span/location, we could highlight the full ;;; section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this but I think the error is fine as is

@RobinMalfait RobinMalfait force-pushed the fix/discard-invalid-decls branch from 5ba4703 to 0309756 Compare January 31, 2025 11:02
@RobinMalfait RobinMalfait marked this pull request as ready for review January 31, 2025 11:02
@RobinMalfait RobinMalfait requested a review from a team as a code owner January 31, 2025 11:02
@RobinMalfait RobinMalfait force-pushed the fix/discard-invalid-decls branch from 459f274 to ddf7dfb Compare January 31, 2025 14:30
@RobinMalfait RobinMalfait force-pushed the fix/discard-invalid-decls branch from ddf7dfb to 7d8c054 Compare January 31, 2025 14:40
@thecrypticace
Copy link
Contributor Author

Can't approve the PR since I opened it originally lol but LGTM 👍

@RobinMalfait RobinMalfait enabled auto-merge (squash) January 31, 2025 14:52
@RobinMalfait RobinMalfait merged commit 35a5e8c into main Jan 31, 2025
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/discard-invalid-decls branch January 31, 2025 14:53
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