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: include script and svelte:options attributes in ast #11241

Merged
merged 5 commits into from
Apr 20, 2024

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Apr 19, 2024

In Svelte 4 AST, script tags (root.instance and root.module) do not have attributes, and prettier-plugin-svelte has to extract them using regex (https://github.com/sveltejs/prettier-plugin-svelte/blob/b120c4de5faa6ca6fa0c6867d6048e1dd1b958ca/src/lib/extractAttributes.ts).

With Svelte 5 (modern AST), the same now applies to svelte:options tag. However, unlike script, it can also have expression attributes:

<!-- only string attributes are allowed on `script` -->
<script lang="ts" context="module"></script>
<!-- svelte:options allows expressions in attributes -->
<svelte:options immutable={true}>

Currently, extractAttributes can only handle string attributes. If we included attributes for these elements in AST, we could remove this function altogether, instead of improving it to also parse expression attributes.

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

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Apr 19, 2024

🦋 Changeset detected

Latest commit: 4299721

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

this seems very sensible to me — @dummdidumm any thoughts on this from a language-tools perspective?

packages/svelte/src/compiler/types/template.d.ts Outdated Show resolved Hide resolved
packages/svelte/src/compiler/types/template.d.ts Outdated Show resolved Hide resolved
@dummdidumm
Copy link
Member

dummdidumm commented Apr 19, 2024

We can't allow expression attributes on the script tag because generics accepts a typescript generic, which could look like this generics="T extends { foo: number}" which should be treated literally and not as an expression.
Adding the attributes itself would probably be ok, though the regex to blank script/style tags would probably not benefit from it (because that needs to happen before the parser).
Maybe it helps to open a very drafty PR over at prettier-plugin-svelte so we can more clearly see what the potential benefits are

@Rich-Harris
Copy link
Member

which should be treated literally and not as an expression

But we could do that, no? The parser would treat that as a text attribute regardless of the {...} because it's a <script>

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 19, 2024

We can't allow expression attributes on the script tag because generics accepts a typescript generic, which could look like this generics="T extends { foo: number}" which should be treated literally and not as an expression.

I've added a test for that, and it seems it's trated literally:

{
	"type": "Attribute",
	"start": 132,
	"end": 168,
	"name": "generics",
	"value": [
		{
			"start": 142,
			"end": 167,
			"type": "Text",
			"raw": "T extends { foo: number }",
			"data": "T extends { foo: number }"
		}
	]
}

Maybe it helps to open a very drafty PR over at prettier-plugin-svelte so we can more clearly see what the potential benefits are

I'll check how it works in sveltejs/prettier-plugin-svelte#436.

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 19, 2024

Maybe it helps to open a very drafty PR over at prettier-plugin-svelte so we can more clearly see what the potential benefits are

I'll check how it works in sveltejs/prettier-plugin-svelte#436.

It worked, I was able to delete extractAttributes.ts and remove lines where it was used. It also fixed the test that were failing because expressions in svelte:options weren't correctly parsed. Can't push that unfortunately, because I had to use file:../svelte/packages/svelte version constraint for svelte.

@dummdidumm
Copy link
Member

Push it anyway 😁 it's fine if the PR fails in CI for a few days, we just don't merge until this goes in.

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 19, 2024

@Rich-Harris
Copy link
Member

thank you!

@jrmajor jrmajor deleted the ast-attributes branch April 20, 2024 13:45
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