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

Adding support for CSS modules #38

Merged
merged 5 commits into from
Mar 12, 2020
Merged

Adding support for CSS modules #38

merged 5 commits into from
Mar 12, 2020

Conversation

jakearchibald
Copy link
Contributor

I think this gets us a step closer to a decent CSS build system, but still feels some way off.

@argyleink @una is there a CSS build system you're particularly happy with, both in terms of developer ergonomics and performance? We never really got there with the devsummit site, so I'm interested to hear about prior art rather than totally reinventing something.

@github-actions
Copy link

github-actions bot commented Mar 10, 2020

🚀 Deploy preview for e670622:
https://tooling-report--698566fd0a9c2004.web.app
(Thu, 12 Mar 2020 15:40:30 GMT)

firebase-preview-action

@@ -51,15 +51,16 @@ This style of import can only be used in `static-build`.
## CSS

```js
import cssURL, { inline } from 'css:./styles.css';
import cssURL, { inline, $tabButton } from './styles.css';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to drop the use of css: prefix 😢

If the import is css:./styles.css, there's no way to tell typescript to treat ./styles.css as a relative path. I'll file an issue with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


- `cssURL` - URL to the CSS resource.
- `inline` - The text of the CSS.

The CSS can also use Sass-style nesting.
- `$*` - Other imports starting with \$ refer to class names within the CSS. So, if the CSS contains `.tab-button`, then `$tabButton` will be one of the exports.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm prefixing all these exports with $ for two reasons. It avoids clashes with JS reserved words, so .return becomes $return, and it means we can add other exports that won't clash.

Happy to change the prefix if there's a preference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should prefix them with 🎨

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a real possibility? (Because I'm all for that) -- but aren't there emoji clashes in Linux?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically it should work, but we’re honestly just asking for trouble and it’s a ball-ache to type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminds me of this from justin w3c/csswg-drafts#3714


return {
name: 'css',
buildStart() {
async buildStart() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all the CSS processing to buildStart so the css.d.ts files are generated before TypeScript gets there. It does mean that we might build CSS files that aren't referenced anywhere, but that should be rare.

@una
Copy link
Contributor

una commented Mar 10, 2020

I think this is a great step in the right direction! Thanks Jake 😄

Here's a branch with the change applied to the styled footer PR for exploration/ref https://github.com/GoogleChromeLabs/tooling.report/tree/styled-footer-css-modules (I'll update it once this is merged)

One thing to note is that this doesn't allow component-based style scope by default (i.e. we will still need to add a class to the parent to scope styles). We'll want to do this anyway to benefit from the code splitting this allows.

To summarize our chat, next steps will be to resolve what to do with component code so we don't have a bunch of floating code blocks per each component. Things to consider:

  • persistent components (header/footer)
  • views based on pages (summary cards)
  • dynamic components (appear on the page after initial load with some user interaction -- do we have any of these?)

Screen Shot 2020-03-10 at 10 42 40 AM

Initial thoughts are to inline critical CSS in the head, then have a global/persistent CSS file at the bottom of the document with all persistent components, then use inline styles injected into a unique file per-page. We can also do a style injection when X pixels above a component (i've seen this before but feels like the scroll listener will be heavier than the CSS savings). I'm not aware of any dynamic components in the design that won't be there on initial page load so I think page-level would suffice, but let me know if I'm wrong there.

cc @argyleink

@jakearchibald
Copy link
Contributor Author

One thing to note is that this doesn't allow component-based style scope by default (i.e. we will still need to add a class to the parent to scope styles).

I thought it did that by renaming all the classes? Or is there some other mechanism?

We can also do a style injection when X pixels above a component (i've seen this before but feels like the scroll listener will be heavier than the CSS savings).

Yeah, I think that'll be too heavy. Also, those things tend to mess up links that point half way down the page.

I'm not aware of any dynamic components in the design that won't be there on initial page load so I think page-level would suffice, but let me know if I'm wrong there.

Oddly, I think dynamic CSS would be pretty easy, since we'd just inline it in the component JS and append it to the page on load. But yeah, I don't think we have any dynamic components planned.

@una
Copy link
Contributor

una commented Mar 10, 2020

I thought it did that by renaming all the classes? Or is there some other mechanism?

This does work when you add classes -- it doesn't scope without them (which is what I meant by "default"), i.e. styling a li within the footer component styles all li's on the page. When I've used CSS Modules in the past, everything is scoped to the component so there wouldn't be a conflict.

It's not a huge issue as long as we're aware

@jakearchibald
Copy link
Contributor Author

CSS Modules in the past, everything is scoped to the component so there wouldn't be a conflict.

Ohh, do you know which implementation of CSS modules that was? I don't think Webpack does that.

@argyleink
Copy link
Member

this is cool:

  • free naming: clobberless dx
  • free scoping: thx to free naming (on us to figure out how to stay dry)
  • option to get raw styles from file, makes for straight forward manual splitting, we get to choose
  • postcss: we still get nesting syntax and the option to bring in more if we want

minimal and unblocks us. seems like it doesnt stop us from making other choices 👍

i guess the only reason to not take it would be if we wanted to hand role an atomic stylesheet, like a hand rolled tailwind? the purist in me wants to do that, but the realist says we can succeed with both, and this work is good. aka the tradeoffs are splitting hairs, and we're fortunate to be stuck between 2 nice options.

jake, did i miss any of the css dx conveniences we have available in this PR? thx for the markdown additions too 👍

@jakearchibald
Copy link
Contributor Author

@una

CSS Modules in the past, everything is scoped to the component so there wouldn't be a conflict.

Ohh, do you know which implementation of CSS modules that was? I don't think Webpack does that.

FWIW, Next.js's implementation will throw if you have a rule like ul li { background: green }. It complains that it isn't 'pure', since it doesn't contain a class name that can be made unique.

You can work around it using :global, but then you're explicitly opting out of scoping.

@jakearchibald
Copy link
Contributor Author

@argyleink

jake, did i miss any of the css dx conveniences we have available in this PR?

Nope, that's pretty much it.

@jakearchibald jakearchibald merged commit a9ab3e3 into master Mar 12, 2020
@jakearchibald jakearchibald deleted the css-modules branch March 12, 2020 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants