Finalize v1.0 with @component tag #12

FredKSchott · 2021-11-30T00:51:59Z

Start Date: 11/29/2021
Status: Abandoned

Summary

Abandoned.

Links

Full Rendered Proposal

jonathantneal · 2021-11-30T17:52:11Z

I like the concept. I am naming TBD on @component.

I think there is also a use-case for hoisting just one global <style> from a component. The workaround being :global.

FredKSchott · 2021-11-30T23:34:02Z

Updated with feedback from RFC call, mainly:

Added a callout for Alpine.js @event support, which would not overlap in practice but may be overlap enough in concept that it would be annoying for those users. Looking for feedback from any current Alpine.js + Astro users.
Relaxed the "top-level only" rules so that this could work inside of pages and layouts, where nesting inside of <head> and <body> would be common.
Added more detailed description of how <style>, <style @component>, <script>, and <script @component> would all work. @jonathantneal This should also address your point around how to define and hoist a global style in a component.
Added an alternative around <Script> and <Style> as special Astro components.

FredKSchott · 2021-12-02T23:09:59Z

Updated to remove conflict with #26 RFC proposed behavior around head/html/body

jonathantneal · 2021-12-03T18:00:44Z

If '@import will be resolved using Vite's @import resolve logic', and Vite uses exports then how does one author an export for CSS?

{
  "exports": {
    ".": {
      "import": "./dist/open-props.module.js",
      "style": "./open-props.min.css"
    }
  }
}

Or is that out of scope?

FredKSchott · 2021-12-03T19:18:17Z

I'm not sure I follow, but the idea would be that we would support whatever Vite supports. So if you do @import 'open-props'; I believe Vite resolves that correctly to open-props/open-props.min.css.

The goal here would be to match what would happen in any other CSS file that Vite is bundling today. For example, if you import './foo.css' in a React component and then foo.css has an @import inside of it, Vite should already be doing some resolving and bundling. This proposal says that we would match that behavior in <style @component>

jonathantneal · 2021-12-03T19:31:40Z

So if you do @import 'open-props'; I believe Vite resolves that correctly to open-props/open-props.min.css.

Literally the style export in my hypothetical example? Or did you test this another way with the open-props package?

It did not work for me when I tested it in the latest Astro, because Vite uses exports, not main, module, style, etc. ^reference

Another workaround export had to be added. ^reference

FredKSchott · 2021-12-03T20:09:08Z

Ah gotcha, no I just assumed Vite still fell back to style if there was no style entrypoint in the exports map, but didn't run it myself to confirm (I should have!).

Looking at that package.json more, and I think the author actually intentionally wants you to do @import 'open-props/style'; (which works in Vite) and intentionally wants @import 'open-props'; to fail. It's hard to say for sure with such a minimal README but that seems to be the intention, in which case, Vite is right to fail on @import 'open-props';.

But even if we assume this to be incorrect behavior of Vite, this specific case is intentional in this RFC: Using Vite to handle resolution in all cases is proposed as objectively better than using Vite to handle resolution in some cases AND using some custom implementation that we've built ourselves in others.

jonathantneal · 2021-12-03T20:34:24Z

Correct. That was the workaround. The two links are to different versions. I was asking the question because CSS import resolution via exports seemed like undefined behavior. Maybe it’s just something to call out. Vite can resolve, but resolving styles as a kind of export is undefined behavior. I would be very comfortable with something like that. What do you think?

active-rfcs/0000-component-tag.md

matthewp · 2021-12-03T20:42:26Z

active-rfcs/0000-component-tag.md

+```
+
+- Pro: No user has an expectation about how this would work. That means less conflict with how `<style>` and `<script>` are expected to work.
+- Con: Requires extra import boilerplate in every component. We could make these globally available to mitigate, but we've never done that before.


We have one global component, Fragment. I think the primary problem with a Component is that it can be renamed (const MyStyle = Style) and its hard to warn against doing that.

matthewp · 2021-12-03T20:42:54Z

Not much to add, 👍 overall, would love to bikeshed on the name but that can come later.

FredKSchott · 2021-12-06T22:16:22Z

Correct. That was the workaround. The two links are to different versions. I was asking the question because CSS import resolution via exports seemed like undefined behavior. Maybe it’s just something to call out. Vite can resolve, but resolving styles as a kind of export is undefined behavior. I would be very comfortable with something like that. What do you think?

I'd love to continue this convo outside of this RFC, since that is already the behavior of a CSS import in Astro today, and this RFC isn't meaning to change anything about that behavior. This line was only added to clarify how resolved CSS imports work today to the reader. I'll make an edit to clarify this in the RFC.

tony-sull · 2021-12-07T20:06:31Z

The distinction between layouts and components really feels like an important one here with regards to the default behavior

I could see it making sense to say layouts would default to not scoping, for example, but components really probably want scoping by default. In either case, default behavior for preprocessing like SASS may be a project-wide default where if I use SASS I'd expect both layouts and components to be run through SASS

If we end up with special behavior for the src/layouts directory, would it make sense to assume scoping by default in src/pages and src/components?

tony-sull · 2021-12-07T20:13:23Z

One other thought here, the idea of an all-or-nothing approach to the style/script processing magic could be either a bit limiting or cumbersome if we end up with a long list of custom attributes to disable each step of the magic

i.e. the RFC allows for <style @component is:global lang='scss'> to get global, bundled, SASS-compiled styles. That's already a bit of a heavy lift as far as DX goes with so many non-standard attributes there, and we could end up needing more down the road if more style processing magic is added to the @component functionality

I know the idea of something similar to svelte-preprocess came up before and was punted, but maybe we'd be better off here including the current default preprocessing behavior with as few attribute flags as possible and know that down the road this behavior would likely just become a default config that could be overridden with something along the lines of astro-preprocess?

jasikpark · 2021-12-07T20:26:46Z

Thoughts about #12

I want to have default scoping for components b/c I don't want their styles to leak into other components unless intentionally, it's rare that I would be using <style> inside a component w/o @component added in this RFC
I've generally used styles as scoped generally + <link rel="stylesheet" src={Astro.resolve("../theme.css")}/> or via import "@theme/theme.css" when I wanted global css to be included in my HeadCommon.astro component, ex: HeadCommon.astro
Would it be possible to add a please:don't:preprocess:me directive to style and script tags that enables this no-magic functionality? So you add an attribute to these elements and they become fully static?
A use case that I would not want to lose is scoped styles + define:vars, which it sounds like this RFC removes w/ scoped styles being behind @component + define:vars only supported via static markup. Is this not a use case that's supported today?
Would it be possible to create this RFC in a way that isn't a binary on-off of no processing to all the processing but rather having a default of no processing + incrementally adding processing as you add directives? I suppose that makes @import resolving ambiguous about whether it's resolved by Vite or the browser, but that could be solved by something like hoist to opt-in to bundling of the css.
Is it a requirement that all attributes are dropped from a tag with @component? Could those attributes not be copied over to the browser?

matthewp · 2021-12-07T20:52:43Z

@jasikpark

A use case that I would not want to lose is scoped styles + define:vars, which it sounds like this RFC removes w/ scoped styles being behind @component + define:vars only supported via static markup. Is this not a use case that's supported today?

This can't be supported in SSR due to the fact that it makes those style tags be runtime dynamic. So we could never optimize these styles and bundle them.

If we had a lighter weight way to do scoping (rather than running PostCSS through Vite) so that it could be done at runtime then perhaps define:vars + scoping could be done together. For that I would propose an opt-in way to do scoping (astro:scope or some such).

FredKSchott · 2021-12-09T17:19:45Z

Hey all, thanks again for the feedback! I'm going to go ahead and split this into two RFCs which should hopefully make it easier to discuss. I'll make sure all feedback here is carried into those PRs so that nothing is lost.

This reverts commit bd4561e.

FredKSchott · 2021-12-09T17:25:32Z

New RFC, focused only on behavior: #41

FredKSchott self-assigned this Dec 1, 2021

matthewp reviewed Dec 3, 2021

View reviewed changes

active-rfcs/0000-component-tag.md Show resolved Hide resolved

matthewp reviewed Dec 3, 2021

View reviewed changes

FredKSchott added 12 commits December 9, 2021 16:23

Create 0000-component-tag.md

478b92b

Update 0000-component-tag.md

bac6c47

Update 0000-component-tag.md

2571ee1

Update 0000-component-tag.md

1d970fa

Update 0000-component-tag.md

770ca33

Update 0000-component-tag.md

57a70a9

Update 0000-component-tag.md

73071da

Update 0000-component-tag.md

24c5002

Update 0000-component-tag.md

1cc82a6

Update 0000-component-tag.md

1b19130

Update 0000-component-tag.md

838f94b

refocus RFC on documenting behavior without API change

bd4561e

FredKSchott force-pushed the FredKSchott-patch-1 branch from 3433db4 to bd4561e Compare December 9, 2021 17:16

FredKSchott changed the title ~~Finalize v1.0 style and script API for components~~ Finalize v1.0 script and style behavior for Astro components Dec 9, 2021

FredKSchott closed this Dec 9, 2021

FredKSchott changed the title ~~Finalize v1.0 script and style behavior for Astro components~~ Finalize v1.0 with @component tag Dec 9, 2021

FredKSchott reopened this Dec 9, 2021

Revert "refocus RFC on documenting behavior without API change"

6015b4d

This reverts commit bd4561e.

FredKSchott closed this Dec 9, 2021

FredKSchott deleted the FredKSchott-patch-1 branch December 9, 2021 17:22

FredKSchott mentioned this pull request Dec 9, 2021

finalize v1.0 component script and style behavior #41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize v1.0 with @component tag #12

Finalize v1.0 with @component tag #12

FredKSchott commented Nov 30, 2021 •

edited

Loading

jonathantneal commented Nov 30, 2021

FredKSchott commented Nov 30, 2021 •

edited

Loading

FredKSchott commented Dec 2, 2021

jonathantneal commented Dec 3, 2021

FredKSchott commented Dec 3, 2021

jonathantneal commented Dec 3, 2021

FredKSchott commented Dec 3, 2021

jonathantneal commented Dec 3, 2021 •

edited

Loading

matthewp Dec 3, 2021

matthewp commented Dec 3, 2021

FredKSchott commented Dec 6, 2021 •

edited

Loading

tony-sull commented Dec 7, 2021

tony-sull commented Dec 7, 2021

jasikpark commented Dec 7, 2021

matthewp commented Dec 7, 2021

FredKSchott commented Dec 9, 2021

FredKSchott commented Dec 9, 2021

Finalize v1.0 with @component tag #12

Finalize v1.0 with @component tag #12

Conversation

FredKSchott commented Nov 30, 2021 • edited Loading

Summary

Links

jonathantneal commented Nov 30, 2021

FredKSchott commented Nov 30, 2021 • edited Loading

FredKSchott commented Dec 2, 2021

jonathantneal commented Dec 3, 2021

FredKSchott commented Dec 3, 2021

jonathantneal commented Dec 3, 2021

FredKSchott commented Dec 3, 2021

jonathantneal commented Dec 3, 2021 • edited Loading

matthewp Dec 3, 2021

Choose a reason for hiding this comment

matthewp commented Dec 3, 2021

FredKSchott commented Dec 6, 2021 • edited Loading

tony-sull commented Dec 7, 2021

tony-sull commented Dec 7, 2021

jasikpark commented Dec 7, 2021

matthewp commented Dec 7, 2021

FredKSchott commented Dec 9, 2021

FredKSchott commented Dec 9, 2021

FredKSchott commented Nov 30, 2021 •

edited

Loading

FredKSchott commented Nov 30, 2021 •

edited

Loading

jonathantneal commented Dec 3, 2021 •

edited

Loading

FredKSchott commented Dec 6, 2021 •

edited

Loading