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

feat: CSS language plugin #2

Merged
merged 25 commits into from
Nov 6, 2024
Merged

feat: CSS language plugin #2

merged 25 commits into from
Nov 6, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Oct 25, 2024

This is the first commit for the CSS language plugin. It implements the basic functionality along with the general infrastructure necessary for the plugin.

I've included just one rule to ensure that the language plugin works. We can add additional rules on a one-off basis after this initial PR is merged.

@nzakas
Copy link
Member Author

nzakas commented Nov 1, 2024

One thing I've noticed about this parser: it is very fault-tolerant, just like the CSS parser built into the browser. That means something like a { is parsed fine without an errors and produces a valid AST even though the syntax is not correct. 🤔

csstree/csstree#301

@romainmenke
Copy link

a { is actually correct syntax.

https://drafts.csswg.org/css-syntax/#consume-simple-block

<eof-token>
ending token
Discard a token from input. Return block.

Encountering an <eof-token> is handled the same as encountering the ending token for a block.

@nzakas
Copy link
Member Author

nzakas commented Nov 1, 2024

@romainmenke it may be correct syntax by the spec, but users would expect a linter to pick that up.

@romainmenke
Copy link

Sure, no disagreement there :)

@nzakas
Copy link
Member Author

nzakas commented Nov 1, 2024

I've updated to catch the errors that csstree does report. Waiting to hear back on a {.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Nov 4, 2024
nzakas and others added 13 commits November 4, 2024 11:26
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@nzakas
Copy link
Member Author

nzakas commented Nov 4, 2024

CSSTree updated Blocks so that curly braces are now included in the location, meaning I could remove the extra CSSSourceCode methods for updating Block locations. 🎉

nzakas and others added 4 commits November 5, 2024 10:22
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Last few suggestions, then LGTM.

Comment on lines +285 to +288
if (Array.isArray(child)) {
child.forEach(grandchild => {
visit(grandchild, node);
});
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed that all array items are always AST nodes, not e.g., null as can be the case in ESTree (for example, in ArrayExpression#elements)?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, yes.

nzakas and others added 3 commits November 5, 2024 15:43
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I'm not sure if the a { parsing is a blocker for merging this and releasing the first version of this plugin. If it isn't, I think this is ready to merge & release.

@nzakas
Copy link
Member Author

nzakas commented Nov 5, 2024

It shouldn't be a blocker. I just left it in there to remind me to follow up.

@nzakas nzakas merged commit 52d4f2c into main Nov 6, 2024
15 checks passed
@nzakas nzakas deleted the plugin-start branch November 6, 2024 21:13
@github-actions github-actions bot mentioned this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants