Add lint rule for implicit is:inline on script tags.
#970
Conversation
🦋 Changeset detectedLatest commit: b458421 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Happydev <81974850+MoustaphaDev@users.noreply.github.com>
Co-authored-by: Happydev <81974850+MoustaphaDev@users.noreply.github.com>
There was a problem hiding this comment.
I think this is a great idea to help people understand the effect of attributes in their script tags! Some thoughts on the wordingt for you @lilnasy ! 🙌
internal/transform/transform.go
Outdated
| } | ||
| h.AppendHint(&loc.ErrorWithRange{ | ||
| Code: loc.HINT, | ||
| Text: "Astro processes your script tags to allow using TypeScript and npm packages, and to optimize browser performance.\n\nAttributes cannot be used on Astro-processed script tags. Therefore, this script tag will be treated as if it has the `is:inline` directive, opting it out of the processing steps and its features.\n\nFor clarity, you might want to add the `is:inline` directive explicitly.\n\nSee docs for more details: https://docs.astro.build/en/guides/client-side-scripts/#script-processing.", |
There was a problem hiding this comment.
| Text: "Astro processes your script tags to allow using TypeScript and npm packages, and to optimize browser performance.\n\nAttributes cannot be used on Astro-processed script tags. Therefore, this script tag will be treated as if it has the `is:inline` directive, opting it out of the processing steps and its features.\n\nFor clarity, you might want to add the `is:inline` directive explicitly.\n\nSee docs for more details: https://docs.astro.build/en/guides/client-side-scripts/#script-processing.", | |
| Text: "This script contains an attribute (ex: `type="module"`, `type="text/partytown"`, `async`) or a directive (`define:vars`) and will not be processed and bundled for optimized browser performance.\n\nThis script will be treated as if it has the `is:inline` directive and therefore features that require processing (e.g. using TypeScript or npm packages in the script) are unavailable.\n\nSee docs for more details: https://docs.astro.build/en/guides/client-side-scripts/#script-processing.", |
This felt maybe a little "life story" for a helpful hint message? If this is a warning message shown when an attribute exists (because people might not realize it's gonna be inline), then I'd frame the ideas in something like the order above:
- why this message is being shown (we found an attribute/directive!)
- what that means (no processing/optimizing for you!)
- why it means that/how they can understand why this is happening (it's treated like inline!)
- any other important information to know (no other features, too. See the docs)
I'm also not sure why the recommendation to add the is:inline directive explicitly. What's the benefit? What kind of clarity are you going for? Just to remind yourself that this one will be treated as inline? Unless there's a strong reason to suggest that I'm not seeing, I'd recommend removing.
There was a problem hiding this comment.
From Sarah's suggestion:
This script contains an attribute (ex:
type="module",type="text/partytown",async) or a directive (define:vars) and ...
Instead of giving examples from the original RFC that might confuse users, we could make this hint dynamic and include current attributes.
There was a problem hiding this comment.
you might want to add the
is:inlinedirective explicitly
I think this information is valuable as it shows you how to disable the hint (and remove the three dots in the editor view)
There was a problem hiding this comment.
shows you how to disable the hint
That's exactly right. It was also part of the RFC that introduced is:inline.
I'm also not sure why the recommendation to add the is:inline directive explicitly.
I guess we wouldn't want to encourage is:inline, because it is almost always an accident. Is that what you're getting at?
There was a problem hiding this comment.
I was getting at, I have no idea why this is a recommendation for this case, if it's already treated as inline. Why would you bother adding it?
Martin's response that adding it will silence the hint/message is a valid reason, so if it's helpful then something like:
Adding the
is:inlineexplicitly will silence this hint.
BUT, that seems like maybe kind of terrible DX -- you have to go back and add this everywhere or you're going to get editor notation yelling at you for things that are just "how things work"?
There was a problem hiding this comment.
I should clarify, the motivation behind this PR is to add friction to the cases where is:inline is added implicitly.
It is usually accidental, it makes the scripts behave drastically differently, and is only required in very specific use-cases.
Moreover, this "discouragement" was part of the original RFC from years ago.
#965 adds an additional warning when a |
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
|
Just checking, because I don't know the required syntax here, but in some of these there are two spaces, and in another, only one. Just in case that matters! |
|
I'll add that @lilnasy did this pull request live with other community members. I had a lot of fun watching him do it and learned a lot. |
|
@sarah11918 Good eye, I hadn't noticed! Go has formatting built-in, it will take care of it on its own sooner or later. Doesn't affect behavior. |
Changes
Testing
Added
scripts/isinline-hint.ts.Docs
Would like docs review on the message.