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

Webpack loader to extract the CSS to a .css file #86

Closed
Keats opened this Issue Dec 29, 2016 · 17 comments

Comments

Projects
None yet
5 participants
@Keats
Copy link

Keats commented Dec 29, 2016

Is it planned? It doesn't seem too hard as there is the getStyle method already

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Dec 29, 2016

Is it planned?

Not yet. Key reason :

  • Static HTML + CSS assets are easily built already (you don't need a webpack loader to create static html files with react, and you don't need a webpack loader to create static css files with typestyle). http://typestyle.io/#/server We've built pdf templates and email templates with just typestyle / react and its been a joy ❤️

  • Also people are free to use calls to style in dynamic code, that is not something that can be seperated out statically in CSS files. e.g. quite commonly designs require code driven media breakpoints. Such CSS isn't resolved till a complete render is done at least once (all drawing functions have executed).

I think the corelation of Dynamic HTML => no loader , Dynamic CSS => no loader explains it well.

Neverthless, tracking in our core upstream package : blakeembrey/free-style#38 to figure out this hard problem (just like it solved other hard problems for us), but I doubt it will be done without sacrificing power 🌹

@basarat basarat closed this Dec 29, 2016

@notoriousb1t

This comment has been minimized.

Copy link
Contributor

notoriousb1t commented Dec 29, 2016

I started an experiment for this about a month ago but for a general cli tool. (uses ts-node to setup an environment, requires the entry file, and then calls getStyles() to output to a .css file. It is probably in a non-working state, but I think that it might be possible to do it. As @basarat mentioned though, you would lose a lot of the dynamic nature of TypeStyle by building such a thing.

https://github.com/notoriousb1t/typestyle-cli

@Keats

This comment has been minimized.

Copy link
Author

Keats commented Dec 30, 2016

I'm not sure what you mean by dynamic CSS or code driven breakpoints (never heard of that before, breakpoints are usually content based).

Looking at the code it seems that calling style will force a full hashing of all the classes and re-append everything to the style tag, which means using it in a component would be a pretty bad idea so most users will define all the classes outside of them like in the docs?
A script could be made to extract the CSS but style would still append all the styles in the tag which is unacceptable imo (other than for emails)

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Dec 30, 2016

code driven breakpoints (never heard of that before, breakpoints are usually content based).

Yup. Content based. And changed in code based on the content e.g. <ResponsiveGrid breakpoint={480}>{somecontent}</ResponsiveGrid>. This is needed when you give your components to someone else and they get to choose the breakpoint.

calling style will force a full hashing of all the classes and re-append everything to the style tag

re-append everything to the style tag : Its pretty fast ~ 50ms. But its not done always. If the output is the same its not rewritten. Thanks to deduping http://typestyle.io/#/advanced/concept-deduping. I personally generally try to call it outside render but have been known to do it in code if the styles are driven by arguments / props passed in.

A script could be made to extract the CSS but style would still append all the styles in the tag which is unacceptable imo (other than for emails)

Agreed. You could wrap style in myStyle and remove calls to style depending on your build.

@Keats

This comment has been minimized.

Copy link
Author

Keats commented Dec 30, 2016

I guess I was hoping typestyle was more like a compiler plugin that would basically eliminate all runtime costs: it would calculate the hash for all style calls, replace the style call by the hash (and remove all typestyle related imports) and write the resulting css in a given file.
You lose dynamic use of it (or have an option to disable that compilation step to keep like right now) but you don't pay any runtime cost/dependencies in prod and you get a separate css file.

No clue if that's doable in TS though

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Dec 30, 2016

@Keats how about npm install something that exposes the same API as typestyle.

  • With prod build it makes all the core functions a noop.
  • And also provides a webpack plugin that can be used to extract all the css that would be generated by calls to style/cssRaw/cssRule.

Nonetheless not on my radar right now.

@pitaj

This comment has been minimized.

Copy link

pitaj commented Dec 30, 2016

@basarat 50ms is stupid slow. 16ms is 60fps. Anything that takes longer, especially if it is a single function call, is unacceptable. It can only be worse on mobile.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Dec 30, 2016

I said that to be safe. Its more like 4ms

image

Just did a test running our application ❤️

@pitaj

This comment has been minimized.

Copy link

pitaj commented Dec 30, 2016

@basarat I can't speak to how representative a benchmark your app is, but the deal is this: the hash and CSS rendering is unnecessary load on a client that can be avoided by compiling it beforehand.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Dec 30, 2016

There is a tradeoff. Writing html by hand is also faster than using React for it.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Dec 30, 2016

the hash and CSS rendering is unnecessary load on a client that can be avoided by compiling it beforehand.

I do get your point though. Just that you lose dynamic code paths this way and the performance gain doesn't seem significant 🌹

@pitaj

This comment has been minimized.

Copy link

pitaj commented Dec 30, 2016

There is a tradeoff

There doesn't have to be

you lose dynamic code paths

Crafting CSS during component creation is an anti-pattern. You should be using different classes for different visual states, not modifying an existing class programmatically.

performance gain doesn't seem significant

Significance is subjective in this case. Those 4ms (or maybe longer, especially on mobile devices) could be the difference between 60fps and 40fps.

@Keats

This comment has been minimized.

Copy link
Author

Keats commented Dec 30, 2016

I agree with @pitaj, it would be a no-brainer if it was a compilation step imo. I can't find if there is a way to transform the AST in a plugin in TS though

@DanielRosenwasser

This comment has been minimized.

Copy link

DanielRosenwasser commented Dec 30, 2016

There isn't a way now - we think that in 2.2 there should also be a way to consume the transform pipeline and rewrite trees as needed - but there won't be an extension point yet because we need to scope out the API.

@Keats

This comment has been minimized.

Copy link
Author

Keats commented Dec 30, 2016

Ah that's a shame. Would something like what we mentioned be in that scope?

@DanielRosenwasser

This comment has been minimized.

Copy link

DanielRosenwasser commented Dec 30, 2016

Actually, now that I reread this issue, this isn't about generating new TS code; you can just as easily use the compiler API to grab the ASTs and generate CSS when you can figure it out.

@Keats

This comment has been minimized.

Copy link
Author

Keats commented Dec 30, 2016

But you also want to remove the existing calls to style since they aren't needed anymore so that means having a pre-transpilation phase, something like:
Parsing -> [Plugins modifying AST (deleting import nodes, replacing style call nodes with a string, ...)/Generating CSS/etc] -> Transpilation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment