-
Notifications
You must be signed in to change notification settings - Fork 30
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
finalize v1.0 component script and style behavior #41
Conversation
Capturing some other feedback from @jasikpark in #12:
@jasikpark @matthewp take a look at https://github.com/withastro/rfcs/blob/style-script-rfc/active-rfcs/0000-style-script-behavior.md#deep-dive-definevars - I believe that we can keep
It is currently the case that all attributes are dropped from |
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.
This looks good as a way to solidify what the current behavior is so we can work off of it towards a new rfc!
- ESM import will not be resolved or bundled, sent untouched/raw to the browser. | ||
- Will be output in the final component render(), useful for adding global JS. | ||
- May be duplicated if this component appears on the page multiple times. | ||
- In the future, we could add some kind of of `is:unique` support to prevent duplicate scripts. |
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.
Isn't this provided by hoist
?
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.
yes, good point. This is trying to say that we could do is:unique
seperately from hoist. But I guess none of that is really in scope for this RFC anymore, so maybe best to remove entirely.
### New RFC Behavior | ||
|
||
- `<script hoist>` contents must be static, therefore `define:vars` is not supported. Because the script is bundled ahead-of-time, dynamic values won't exist. This is currently easy to break in v0.21, so this RFC proposes removing the support entirely for `<script hoist>` (still available for `<script>` and `<style>`). | ||
- Cannot be nested within a template expression or conditional. Bundled scripts must be scanned by the compiler. This is currently easy to break in v0.21 with something like `{alwaysFalse && <script hoist>...` so this RFC moves to remove support for this. This change shouldn't impact many users. |
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.
How can we test for whether it will impact many users? Once usecase I would personally use for supporting LaTeX formatting would be conditionally adding in the CSS and JS for processing LaTeX markup only on pages with "hasMath" in the front-matter. I don't know how often that's a thing, but I definitely think it's a valid usecase!
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.
Ah you're right, this may work given how the bundler works today. I'd assumed most use-cases for this were buggy. Let me address this in the RFC, but regardless this will be a pretty strict requirement for the build perf + SSR-readiness improvements that we need to make.
There could be other ways to solve this though, by treating that logic more like a component to be added conditionally.
Co-authored-by: Caleb Jasik <[email protected]>
Co-authored-by: Caleb Jasik <[email protected]>
Summary
Finalize the component styling & scripting behavior for v1.0. Documents existing behavior and adds 2-3 new restrictions to prevent bugs.
Links