-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Script is:inline clarifications #1568
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice job 🙌
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Note to self: clarify that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Dan — just left a suggestion.
Note to self: clarify that
type=module
protects the scope of a script, so you need to useis:inline
to add something to the global scope
On this, it is possible to add to the global scope explicity from a type=module
script, but it’s generally considered bad practice because of the async nature of modules (if you can’t control when each script runs, depending on globals across scripts is recipe for trouble).
<!-- Astro bundles this into a type="module" script -->
<script>
window.foo = 'some global value';
</script>
<!-- equivalent inline version -->
<script is:inline>
var foo = 'some global value';
</script>
Basically both of these should be avoided 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Nicely done explaining this clearly without going deep into the weeds 👍
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Everyone reviewed and approved this one, so I'm going to merge and bring any additional changes (regarding the @delucis I'm okay with the wordiness of the bundling explanation for now as it's in an advanced aside, but I may revisit it in that PR. |
What kind of changes does this PR include?
Description
defer
and similar attributes don't work on inlined scriptsdefine:vars
impliesis:inline
.