Added style AST to SvelteStyleElement

[marekdedic]

Fixes #317

[marekdedic]

I think this is actually ready now - currently there are parsers only for CSS and SCSS, but others can easily be added later (I am eyeing LESS and stylus...).

[marekdedic]

This is probably not the right thing to do...

[ota-meshi]

I don't think it's compatible with ESLint yet. The tokens parsed by postcss are not added to the tokens and comments fields.
Since postcss has the information of comments in the node as text in the raws field, it is difficult to correctly extract tokens from the node. So we need to extend the parser. But I don't know if parsers other than postcss can do the same.

Even if I did build an AST for SCSS, I don't think the rules would be able to handle the AST very well because we don't do any preprocessing.
For example, I don't think we can extract the class name from the following SCSS selectors:

@mixin define-emoji($name, $glyph) {
  span.emoji-#{$name} {
    font-family: IconFont;
    font-variant: normal;
    font-weight: normal;
    content: $glyph;
  }
}

@include define-emoji("women-holding-hands", "👭");

https://sass-lang.com/documentation/style-rules

I'm still against having a parsed AST for <style>. That's because I don't have a good idea how to handle CSS flavor ASTs well.
Stylelint stopped parsing non-CSS syntax in core with the release of version 14. This was because it was too costly to maintain.

[marekdedic]

Thanks for the comments :)

I'm still against having a parsed AST for <style>. That's because I don't have a good idea how to handle CSS flavor ASTs well.
Stylelint stopped parsing non-CSS syntax in core with the release of version 14. This was because it was too costly to maintain.

I think that without parsing <style>, features like sveltejs/eslint-plugin-svelte#402 are basically impossible to implement. I agree that non-CSS syntax is a headache. How do you handle non-JS syntax? (I think you specify the parser in eslintrc, right?) Couldn't that solution be reused?

I don't think it's compatible with ESLint yet. The tokens parsed by postcss are not added to the tokens and comments fields.
Since postcss has the information of comments in the node as text in the raws field, it is difficult to correctly extract tokens from the node. So we need to extend the parser. But I don't know if parsers other than postcss can do the same.

Hmm, if I understand this correctly, the tokens and comments fields are not on individual AST nodes, but somewhere "global", right? That seems solvable in principle. Also, PostCSS defines the Comment node, is that not where comments are stored? Or is there something else in the raws field?

So far, I have refrained from any modifications to the parser, I agree that that would be a road to hell... However, I don't see how that's needed, apart from the issue with comments - am I missing something?

And as to your example - you're right, that would make it extremely hard to extract the class name. But the same thing happens with

<script lang="ts">
  export let classValue: string;
</script>
<div class={classValue} />

I would be OK with this, but I understand if you are not...

[ota-meshi]

We need the AST parsed by postcss to handle the sveltejs/eslint-plugin-svelte#402 rule, as you say. But since that AST is only needed to extract the selector, the AST does not need the correct position and does not need to be compatible with the ESLint engine.
Therefore, for non-CSS, I think we can use a processor to convert that to CSS and then use the resulting AST, parsed with postcss, in the rules.

The tokens and thecomments should be collected in the Program node.
https://eslint.org/docs/latest/extend/custom-parsers#the-program-node
Otherwise, APIs that work with tokens will not work properly.
https://eslint.org/docs/latest/extend/custom-rules#contextgetsourcecode

Comment nodes are not generated when the following CSS is parsed by postcss.

a /* comment */ b {
   color: /* comment */ red;
}

[marekdedic]

The tokens and thecomments should be collected in the Program node.
https://eslint.org/docs/latest/extend/custom-parsers#the-program-node
Otherwise, APIs that work with tokens will not work properly.
https://eslint.org/docs/latest/extend/custom-rules#contextgetsourcecode

Hmm, ok, that's an issue, however I think it's a solvable issue.

Therefore, for non-CSS, I think we can use a processor to convert that to CSS and then use the resulting AST, parsed with postcss, in the rules.

Which processor? Postcss? 😄 We'd need to configure that processor the same way postcss needs to be configured in this PR...

We need the AST parsed by postcss to handle the sveltejs/eslint-plugin-svelte#402 rule, as you say. But since that AST is only needed to extract the selector, the AST does not need the correct position and does not need to be compatible with the ESLint engine.

Yeah, you're right that we could just do that (modulo the processor stuff). But then every plugin and every rule touching <style> would need to do the same thing all over again - my motivation behind wanting to pull this into the parser. I think we shouldn't let perfect be the enemy of the good.

---

Ok, I'm willing to do the work to make this compatible with the ESLint AST, fix the tokens and comments stuff and add some additional style languages - but I don't really want to do that if you think this shouldn't be in the parser at all as it would be a waste of time... It's your call, obviously :)

Anyway, thanks for the feedback!

[ota-meshi]

Which processor?

I intended it to mean a "processor" that converts from SCSS, Sass, Less, and Stylus, etc. to CSS.

my motivation behind wanting to pull this into the parser.

It's an idea I just came up with, but it might be better to provide the <style> AST via services instead of including it in the svelte (and script) AST node.
In that case, compatibility with the AST used by ESLint is not required.
typescriptp-eslint/parser can also get typescript generated nodes via services.

https://eslint.org/docs/latest/extend/custom-parsers#parseforeslint-return-object:~:text=the%20AST%20object.-,services,-can%20contain%20any

What do you think?

[marekdedic]

Thanks, I think using the services is a great idea - we wouldn't have to worry about AST compatibility and could still provide the AST to rules in one place.

About the preprocessors - Do you see any advantage to providing the AST of the resulting CSS instead of the original? Because code wise, both mean we have to handle custom style languages, so we have to deal with that complexity either way...

[ota-meshi]

I don't think we can extract the class names used from the following SCSS without using preprocessors.
But that may just be a limitation of the sveltejs/eslint-plugin-svelte#402 rule.

@mixin define-emoji($name, $glyph) {
  span.emoji-#{$name} {
    font-family: IconFont;
    font-variant: normal;
    font-weight: normal;
    content: $glyph;
  }
}

@include define-emoji("women-holding-hands", "👭");

[marekdedic]

Yeah, that's right... :/

On the other hand, most of the time you'd probably want the original AST rather than the transpiled one. We could obviously provide both, but that seems to be a bit overkill...

[changeset-bot]

⚠️ No Changeset found

Latest commit: ab5b55e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[marekdedic]

Obsoleted by #340