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

Make scoped CSS the default in 'template-tags' #1037

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bitxplora
Copy link

@bitxplora bitxplora commented Jul 9, 2024

Making scoped-CSS the default will improve the developer experience by not having to fight spill CSS that is affecting other components.

Make Scoped CSS the default in 'template-tags

Rendered

Summary

Based on my observation while using template-tags to author components, the style tag inside template-tags is global, which I think should not be because it may affect other components. Also, libraries or imported components might have used the same style name, so the issue will become nested and, therefore won't be easy to trace. Even if one works around this using a unique class wrapper for each component, the possibility of collision is still present, and uniqueness is not guaranteed project-wide.

This pull request is proposing a new RFC.

To succeed, it will need to pass into the Exploring Stage), followed by the Accepted Stage.

A Proposed or Exploring RFC may also move to the Closed Stage if it is withdrawn by the author or if it is rejected by the Ember team. This requires an "FCP to Close" period.

An FCP is required before merging this PR to advance to Accepted.

Upon merging this PR, automation will open a draft PR for this RFC to move to the Ready for Released Stage.

Exploring Stage Description

This stage is entered when the Ember team believes the concept described in the RFC should be pursued, but the RFC may still need some more work, discussion, answers to open questions, and/or a champion before it can move to the next stage.

An RFC is moved into Exploring with consensus of the relevant teams. The relevant team expects to spend time helping to refine the proposal. The RFC remains a PR and will have an Exploring label applied.

An Exploring RFC that is successfully completed can move to Accepted with an FCP is required as in the existing process. It may also be moved to Closed with an FCP.

Accepted Stage Description

To move into the "accepted stage" the RFC must have complete prose and have successfully passed through an "FCP to Accept" period in which the community has weighed in and consensus has been achieved on the direction. The relevant teams believe that the proposal is well-specified and ready for implementation. The RFC has a champion within one of the relevant teams.

If there are unanswered questions, we have outlined them and expect that they will be answered before Ready for Release.

When the RFC is accepted, the PR will be merged, and automation will open a new PR to move the RFC to the Ready for Release stage. That PR should be used to track implementation progress and gain consensus to move to the next stage.

Checklist to move to Exploring

  • The team believes the concepts described in the RFC should be pursued.
  • The label S-Proposed is removed from the PR and the label S-Exploring is added.
  • The Ember team is willing to work on the proposal to get it to Accepted

Checklist to move to Accepted

  • This PR has had the Final Comment Period label has been added to start the FCP
  • The RFC is announced in #news-and-announcements in the Ember Discord.
  • The RFC has complete prose, is well-specified and ready for implementation.
    • All sections of the RFC are filled out.
    • Any unanswered questions are outlined and expected to be answered before Ready for Release.
    • "How we teach this?" is sufficiently filled out.
  • The RFC has a champion within one of the relevant teams.
  • The RFC has consensus after the FCP period.

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Jul 9, 2024
@bitxplora bitxplora changed the title RFC to make scoped CSS the default in 'template-tags' Make scoped CSS the default in 'template-tags' Jul 10, 2024
@ef4 ef4 added the S-Exploring In the Exploring RFC Stage label Jul 12, 2024
@ef4
Copy link
Contributor

ef4 commented Jul 12, 2024

(I wrote glimmer-scoped-css so in general I think it's a good pattern.)

I think the important thing this RFC needs to clarify is what we're committing to in published addons. Addons are not supposed to publish template tag, they're supposed to turn it into valid javascript first.

One way that could work is essentially what apps already do now in their own build when using the addon:

// === GJS Input ===
const MyComponent = <template>
  <style>
    h1 { color: red }
  </style>
  <h1>Hello</h1>
</template>

// === JS Output ===
import { template } from '@ember/template-compiler';

/*
  This would be a real file in the published addon containing:
    h1[data-scopedcss-5f78d2d716-402c934e4d] { color: red }
*/
import "./scoped-css-5f78d2d716.css";

const MyComponent = template(`
  <h1 data-scopedcss-5f78d2d716-402c934e4d>Hello</h1>
`);

If that's how we want it to work, we're not committing to anything new in addons, because they're already allowed to import paths ending in .css and expect that to mean "append this CSS to the dom before my module evaluates".

However, one downside of that strategy is that the CSS is eager, in the sense that it goes in the DOM when you import the component, not when you render the component. A component that's imported but never rendered still has its CSS in the DOM forever. Does this matter? Personally I suspect no. But there is an alternative design like:

// === GJS Input ===
const MyComponent = <template>
  <style>
    h1 { color: red }
  </style>
  <h1>Hello</h1>
</template>

// === JS Output ===
import { template } from '@ember/template-compiler';

// this utility would do a reference-counted insert/remove of the constructible stylesheet on the document
import { ensureAppended } from 'glimmer-scoped-css/runtime';

const __stylesheet0__ = new CSSStyleSheet();
__stylesheet0__.replaceSync(`
  h1[data-scopedcss-5f78d2d716-402c934e4d] { color: red }
`);

const MyComponent = template(`
  {{ (ensureAppended __stylessheet0__) }}
  <h1 data-scopedcss-5f78d2d716-402c934e4d>Hello</h1>
`, { 
      scope: () => ({ __stylesheet0__ }) 
    }
);

In one sense, this RFC doesn't really have to decide between these two alternatives, because both only use features that addons are already allowed to use. But it would probably be good for us to offer solid guidance to addon authors so they aren't each picking their own solution.

@ef4
Copy link
Contributor

ef4 commented Jul 12, 2024

Separate topic: I think this RFC needs to actually write out what people are allowed to say in their CSS and exactly how it differs from standard CSS. For example:

  • :deep is a thing
  • what CSS features are not supported? (I think maybe named keyframes?)

This can lean on the documentation for Vue's scoped CSS feature, because that is the implementation I started from.

@bitxplora
Copy link
Author

  1. Within the style tag in the template-tag, one should be able to write the standard CSS, it should be scoped by default.
  2. Also, there are good uses for the capability to import style files. For example:
    • If there is more than one component that needs the same styling, one should be able to have a single CSS file that is importable into only the components that need it.
    • Some components share a common base styling which each component builds upon. Having a common base in a single file that can be imported by the components that need it is excellent.
  3. It is more Svelte than Vue's scoped CSS.

Separate topic: I think this RFC needs to actually write out what people are allowed to say in their CSS and exactly how it differs from standard CSS. For example:

* `:deep` is a thing

* what CSS features are not supported? (I think maybe named keyframes?)

This can lean on the documentation for Vue's scoped CSS feature, because that is the implementation I started from.

Making scoped-CSS the default will improve the developer experience by
not having to fight spill CSS that is affecting other components.
Changed the placeholder, which was 'xyz', for rfc number in the rfc file's name to the
actual rfc number which is 1037.
@gossi
Copy link

gossi commented Jul 14, 2024

I'm also a friend of plain CSS as it is become sooo good lately. However, what about CSS pre-processors?

I could very much imagine the case, where people do use CSS modules (or similar) and want to use compose, or to use some looping behavior from a given preprocessor (SASS or PostCSS).

I think the RFC should answer one of the two questions:

  • If there is a preprocessor infrastructure, how to use it? ie. how does one integrate their preprocessor of choice? (app, addon, engine)
  • If there there is no preprocessor infrastructure, then reasons for why there are none. That is to point people to the RFC for the answer on this topic

@NullVoxPopuli
Copy link
Contributor

However, what about CSS pre-processors?

I don't think addons should publish non-native CSS. Like how we require their JS to be compiled, so should we require their CSS be compiled. If they want variables/etc, they should be native CSS variables.

I could very much imagine the case, where people do use CSS modules (or similar) and want to use compose, or to use some looping behavior from a given preprocessor (SASS or PostCSS).

Since the compile target is ultimately an import of CSS, and since how that import is interpreted must be determined by the app, and thus follow the conventions of whatever packager/app system an app is using (webpack, vite, etc).

Webpack uses loaders, so you could configure all CSS to use sass or postcss: https://webpack.js.org/loaders/postcss-loader/
(there would be no point in css-modules, because the proposed tool here already scopes correctly)

However, via their test, and related, option(s), you could even run these varying loaders over your node_modules.

For Vite, they automatically apply postcss by default: https://vitejs.dev/guide/features.html#css (if detected)

In anycase, if folks want to use CSS modules, there is nothing that this RFC needs to cover.

Both Webpack and Vite support css-modules, and to use them, you'd import x from './styles.module.css', for example, and matching up your x.whatever classes in your gjs/gts.

I don't think it makes sense for any of these features to be implemented in co-located components.
gjs/gts is much easier to support, and conceptually makes more sens.

If webpack or vite are configured to handle multiple file extensions, then we could probably support that with little effort.

<template>
 
  <style lang="sass">...</sytle>
</template>

becomes

import './styles.sass';

once compiled.

Since different tools support different features via query parameters, I think we can use the <style> tag's attributes to add values to the import's query parameters.

for example, using Vite's "inline": https://vitejs.dev/guide/features.html#disabling-css-injection-into-the-page

<template>
 
  <style inline>...</sytle>
</template>

becomes

import './styles.css?inline';

once compiled.

@bitxplora
Copy link
Author

bitxplora commented Jul 16, 2024

When one uses a CSS preprocessor, one will probably be practicing scalable and modular architecture for CSS (SMACSS) and the like.

 scss/
 ├── base/
 │   ├── _reset.scss
 │   └── _typography.scss
 ├── components/
 │   ├── _button.scss
 │   └── _card.scss
 ├── layout/
 │   ├── _header.scss
 │   └── _footer.scss
 └── utilities/
     ├── _variables.scss
     ├── _mixins.scss
     └── _functions.scss

This means one will have the style for each component in its own .scss file in the components directory.
That is, you have to structure your stylesheets to align with your components. Each component should have its own SCSS file, making it easier to maintain and update styles.

So, putting the SCSS in the component's style tags, might not be the best.

At the same, what is suggested by @NullVoxPopuli; passing of the preprocessor as a language attribute of the style tag is a good one.

I'm also a friend of plain CSS as it is become sooo good lately. However, what about CSS pre-processors?

I could very much imagine the case, where people do use CSS modules (or similar) and want to use compose, or to use some looping behavior from a given preprocessor (SASS or PostCSS).

I think the RFC should answer one of the two questions:

* If there is a preprocessor infrastructure, how to use it? ie. how does one integrate their preprocessor of choice? (app, addon, engine)

* If there there is no preprocessor infrastructure, then reasons for why there are none. That is to point people to the RFC for the answer on this topic

@ef4 ef4 removed the S-Exploring In the Exploring RFC Stage label Aug 9, 2024
@ef4 ef4 added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Aug 23, 2024
@ef4
Copy link
Contributor

ef4 commented Aug 30, 2024

A possible amendment that came out of real usage issues at Cardstack:

Because it's easy to unintentionally apply your transforms to third-party code, it's not great that the current implementation repurposes the meaning of <style> in templates. It may be smarter to make the scoped behavior opt-in via <style scoped>, instead of opt-out via <style unscoped>.

@ef4
Copy link
Contributor

ef4 commented Sep 6, 2024

The above comment -- making scoping opt-in instead of opt-out -- is now implemented in glimmer-scoped-css 0.6.0.

@kategengler
Copy link
Member

I haven't followed all the discussion of scoped css, so forgive me if this has been discussed already:

CSS @scope allows scoping css rules to elements. It is not implemented yet in Firefox but is behind a flag. It is a draft spec

<div>
  <style>
    @scope {
      h1 {
          background: blue;
      }
    }
  </style>
  <h1>I'm blue</h1>
</div>
<h1> I'm not blue</h1>

<style scoped was an earlier proposal that was removed.

@ef4
Copy link
Contributor

ef4 commented Sep 7, 2024

I think the proposed CSS @scope is different from glimmer-scoped-css because it's not lexically-scoped. Meaning that it would affect {{yield}}ed content differently.

glimmer-scoped-css doesn't let styles leak down into components that you {{yield}} to. I think @scope would, since it's talking about the DOM subtree and doesn't know anything about component invocation boundaries.

@kategengler
Copy link
Member

@scope rules definitely affects anything beneath it in the tree. I didn't realize that glimmer-scoped-css scopes to literally only the template in the particular component.

@bitxplora
Copy link
Author

@kategengler scoped CSS, in this case, it is scoping to component rather than element, which should be the default. If there is an agreement, that a component is a self-contained UI unit with its style.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Sep 8, 2024

Because it's easy to unintentionally apply your transforms to third-party code, it's not great that the current implementation repurposes the meaning of <style> in templates.

The above comment -- making scoping opt-in instead of opt-out -- is now implemented in glimmer-scoped-css 0.6.0.

@ef4, What are the technical constraints around this?

The idea of requiring an attribute to make socped implies that we want the default to be global and that's what we want folks to be doing in their components (by default)

Is it possible to default to scoped (requiring unscoped or global to opt out) when encountering a <style> tag in a strict-mode component?

@backspace
Copy link

Before 0.6.0 unscoped was the way to opt out, the problem we were having is that the scoping AST transform of the hosting Ember application was being applied to components from an addon which caused build errors from Embroider about missing dependencies. There should be a way to make a transform only apply to one’s own components vs an addon but it wasn’t clear how.

Regardless, with the trends over the years toward explicitness vs “magic”, it also felt fitting to require scoped to opt in. There’s a template lint rule that pushes back toward opting out, in a way, in that you have to disable it to use an unscoped style. If there’s interest maybe that can be moved into the addon.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Sep 9, 2024

transform of the hosting Ember application was being applied to components from an addon which caused build errors from Embroider about missing dependencies. There should be a way to make a transform only apply to one’s own components vs an addon but it wasn’t clear how.

Is it not possible to fix whatever situation makes app-transforms apply to addons?
How do you scope transforms to the app?

, it also felt fitting to require scoped to opt in.

It just means we think people should be writing global CSS by default -- which I think I might strongly disagree with. :-\

At least, I want the pit of Success to be scoped, because that's what folks really want. Global CSS, afaict, is the exception

@kategengler
Copy link
Member

I strongly disagree with making the plain <style> tag do anything differently than it would do in plain html. Making the behavior of <style> different inside <template> would go against what I feel is core Ember: We don't takeover browser features. It would mean you could no longer safely paste html from elsewhere and expect it to behave exactly the same as it does outside of Ember.

The scoped attribute isn't ideal (it was a previous WHATWG proposal), but that would be enough for me to feel we weren't taking over the behavior of html.

@lukemelia
Copy link
Member

I agree with @kategengler about not taking over normal browser behavior. I think a lint rule would be sufficient to nudge everyone in the right direction while preserving default browser behavior.

Regarding the concern of using <style scoped>... because it could be a future browser spec, perhaps we could use <style {{scoped}}>..., evocative of an element modifier?

@kategengler
Copy link
Member

I'm less concerned about <style scoped> since it was a failed proposal, but I guess it could always come back.

@bitxplora
Copy link
Author

The function of <style> will not change, only that the style applies to the component

@kategengler
Copy link
Member

The function of <style> will not change, only that the style applies to the component

If <style> inside of <template> is scoped only to elements within that <template> the behavior of <style> has changed. In html, I can put a <style> tag anywhere and change, for better or worse, styles affecting the entire page.

@bitxplora
Copy link
Author

The inline style attribute can easily be distinguished from the component-level style. If a component is a self-contained UI unit with its style, then global CSS is not the right way.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Sep 9, 2024

Who's putting <style> in vanilla HTML?
There is <link>

@bitxplora
Copy link
Author

It is like having <style> in the of an HTML file, it only applies to that specific file, other files or pages will not be affected. So the <style> functionality remains as it should be.

@lukemelia
Copy link
Member

I've put <style> tags into templates and components before because you can use glimmer to get dynamic css. (Note that this does not work with glimmer-scoped-css since it extracted at build-time.)

@NullVoxPopuli
Copy link
Contributor

Are ya'll using strict mode/gjs/gts when you put <style> into your components? (ignoring HTML for a moment)

@bitxplora
Copy link
Author

bitxplora commented Sep 9, 2024

This is a sample .gjs file;

import { concat } from '@ember/helper';
import tippyTip from '../modifiers/tippyTip';

const tooltip = (transactionType) => {
  if (transactionType === 'fob') transactionType = 'FOB';
  return `Select the currency in which the ${transactionType} is denominated`;
};

<template>
    <div class="brand-text" >
      <label for={{( concat @transactionType "Currency") }}></label>
      <select
        id={{( concat @transactionType "Currency") }}
        name={{( concat @transactionType "Currency") }}
        class="currency-selected" autofocus="" required=""
        {{tippyTip 'mouseenter' 'top' (tooltip @transactionType)}}>
        <option value=''>CUR</option>
          {{#each @currencies as |currency|}}
            <option id={{currency}} name={{currency}} value={{currency}}>{{currency}}</option>
          {{/each}}
      </select>
    </div>
    <style>
      .currency-selected {       
        padding-left: 0.1rem;
        font-size: 0.7rem;
        min-height: 2.0rem;
      }
    </style>
</template>

@bitxplora
Copy link
Author

I believe that styles in the component should not pollute the global CSS

@NullVoxPopuli
Copy link
Contributor

I believe that styles in the component should not pollute the global CSS

aye, same

@NullVoxPopuli
Copy link
Contributor

I'd like to block on on using data attributes for classes instead of classes. <3

styles aren't data, and the closer we stick the the intended semantics the better.

@ef4
Copy link
Contributor

ef4 commented Sep 25, 2024

I believe that styles in the component should not pollute the global CSS

Nobody disagrees that polluting global CSS from a component is bad style. Just like manipulating javascript global variables from a component is bad style. But it would be very bad if we broke the possibility of manipulating javascript global variables from a component, because we'd be breaking Javascript. And I would argue this is the same kind of problem, and making it impossible to manipulate global styles from a component would be breaking HTML.

  • <style></style> in HTML does pollute global styles. Deviating from HTML is risky and confusing.
  • we could use a different syntax that doesn't already have a meaning in HTML, like <ScopedStyle></ScopedStyle>, but this has big implications for tooling. The great benefit of putting CSS inside <style> is that all the parsers and syntax highlighters and formatters already know to handle CSS correctly in that position, and none of them will understand a custom element.

@ijlee2
Copy link
Member

ijlee2 commented Sep 27, 2024

Before making scoped CSS the default, I think there needs to be a first-class support from content-tag that will allow codemods to parse and alter *.{gjs,gts} easily.

Currently, it's up to each codemod author to come up with their own implementation. For example, eslint-plugin-ember depends on ember-eslint-parser. The latter can't be used in @codemod-utils because it's unrelated to eslint and its plugin system.

I think we may head towards a path where migrations become costly, if we don't first solve how to easily write programs that can modify template(s) and class(es) in a single file, before we add styles to the equation.

@bitxplora
Copy link
Author

Before making scoped CSS the default, I think there needs to be a first-class support from content-tag that will allow codemods to parse and alter *.{gjs,gts} easily.

Currently, it's up to each codemod author to come up with their own implementation. For example, eslint-plugin-ember depends on ember-eslint-parser. The latter can't be used in @codemod-utils because it's unrelated to eslint and its plugin system.

I think we may head towards a path where migrations become costly, if we don't first solve how to easily write programs that can modify template(s) and class(es) in a single file, before we add styles to the equation.

I think this has to do with implementation, but RFC is about having a picture of how things should be; a desired state. Policy (RFC) is a picture that guides implementation.

So, for the emberjs component to meet the definition of a component—a unit of user interface with its style and behavior—scoped CSS is needed.

Some of the goodness of scoped CSS:

  • Free from pollution, the global style.
  • Free from accidentally altering the style of other components or templates.

@ef4
Copy link
Contributor

ef4 commented Oct 17, 2024

I was thinking more about the question of whether scoped should be the default, and I realized we haven't discussed an important aspect of the problem: if we went with scoped-by-default it's a breaking change and would require a fairly extensive rollout plan across the ecosystem.

Lots of ember templates already exist and contain <style> and it means unscoped. People may be under the impression that we can limit this to only template tag, but (1) template tag is shipped, so it's still a breaking change, and (2) template tag is just a light syntactic transformation over the javascript representation of templates, so somebody would need to explain what it means at that layer.

Also, we would need an explanation for what's supposed to happen, for example, when people are using the runtime template compiler. I think <style> should mean one consistent thing, and if we decide that means scoped an implementation of scoping would need to get baked fairly deep into glimmer. That's quite different from what glimmer-scoped-css is doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants