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

Full local export / server side rendering #159

Closed
wants to merge 1 commit into from

Conversation

ndreckshage
Copy link

@ndreckshage ndreckshage commented Dec 17, 2016

This (1) removes SSR error to allow style-loader to be used when webpack used for both client & server code. And importantly, it (2) gives option to specify fullLocalExport with style-loader?fullLocalExport to return all content instead of just content.locals. This allows developer to deal with the actual CSS as well as class names. For example, I use higher order components in React to collect critical styles for purpose of initial server side render.

What kind of change does this PR introduce?

This removes the thrown error for SSR, and enables someone to return locals and CSS rather than just class names.

Did you add tests for your changes?

No.

If relevant, did you update the README?

Yes.

Summary

There are numerous discussions on server side rendering.

facebook/create-react-app#172
#109

This PR attempts to solve, but should be closed / rejected because you can't use singletons on server.
#137

Does this PR introduce a breaking change?

No breaking changes. Adds query option.

Other information

@jsf-clabot
Copy link

jsf-clabot commented Dec 17, 2016

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@ndreckshage
Copy link
Author

This pr with https://github.com/HumbleSpark/react-ssr-critical-styles (for example) is a nice option for critical styles with React, using style-loader like normal for all non-server rendered code. Simple alternative to https://github.com/kriasoft/isomorphic-style-loader

@lukewestby
Copy link

@SpaceK33z any thoughts on this?

@SpaceK33z
Copy link
Contributor

@lukewestby sorry I don't use SSR and don't maintain style-loader, so I can't really give a good opinion about this.

@ndreckshage
Copy link
Author

@sokra any chance style-loader could support this? It's a pretty small change and optional with query param. Would be nice if style-loader could return same thing as css-loader.

@joshwiens
Copy link
Member

@ndreckshage - Couple of things.

If you are looking to get ahold of someone, use @webpack-contrib/org-maintainers. Tobias and the rest of the Webpack Core team are focusing only on Webpack and it's core integrations, everything in contrib is handled by that group.

As to this specific topic, it's something that is going to have to be discussed internally. What I can tell you is we will be supporting server side rendering in on form or another.

We have a bit of house keeping to contend with first, I'd like to leave this open and revisit the topic in a few weeks when we have a bit less on our collective plates.

@@ -61,6 +61,11 @@ By default, the style-loader appends `<style>` elements to the end of the `<head

If defined, the style-loader will re-use a single `<style>` element, instead of adding/removing individual elements for each required module. **Note:** this option is on by default in IE9, which has strict limitations on the number of style tags allowed on a page. You can enable or disable it with the singleton query parameter (`?singleton` or `?-singleton`).

#### `fullLocalExport`
Copy link
Member

Choose a reason for hiding this comment

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

fullLocalExport => export/locals/exportLocals

@@ -14,9 +14,10 @@ module.exports.pitch = function(remainingRequest) {
"// load the styles",
"var content = require(" + loaderUtils.stringifyRequest(this, "!!" + remainingRequest) + ");",
"if(typeof content === 'string') content = [[module.id, content, '']];",
"if(content.locals && " + query.fullLocalExport + ") module.exports = content;",
Copy link
Member

Choose a reason for hiding this comment

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

fullLocalExport => export/locals/exportLocals

@@ -1,6 +1,6 @@
{
"name": "style-loader",
"version": "0.13.1",
"version": "0.14.0",
Copy link
Member

Choose a reason for hiding this comment

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

Please don't 😛 . npm version is the reponsibility of the respective maintainer

@@ -21,9 +21,7 @@ var stylesInDom = {},
styleElementsInsertedAtTop = [];

module.exports = function(list, options) {
if(typeof DEBUG !== "undefined" && DEBUG) {
if(typeof document !== "object") throw new Error("The style-loader cannot be used in a non-browser environment");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be removed, we currently don't intend to let this loader work outside of browser context, there is discussion about it going please join that 😛

cc @ekulabuhov @bebraw @d3viant0ne

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, better leave. It should be documented visibly that you are meant to use style-loader in browser environment only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Scratch my comment. I can see the point now. Basically we have to wait for a decision before we can move on the PR.

@michael-ciniawsky
Copy link
Member

@ndreckshage In any case please close and reopen the PR to trigger the CLA Bot again and rebase against current master 😛 .

@michael-ciniawsky
Copy link
Member

@ndreckshage @bebraw General question 😛 , if we 💯 agree upon making this loader browser only, which I'm personally in favour of, would the PR still make sense?

@ndreckshage
Copy link
Author

@michael-ciniawsky if that is agreed, then this pr wouldn't make sense (making style loader work server side is the point of this pr).

but why is that agreed? of course it shouldnt be the responsibility of this to actually inject styles server side - it can do that on client. but to work server side style loader simply needs to be less opinionated than it currently is. what is the reasoning for returning a subset of what css loader returns?

rendering react applications (for example) server side is pretty common, and people use webpack for both the client & server version of that application. why not support that?

however, this doesn't impact me anymore since there are other good options for ssr styles that i prefer over css modules, like styled-jsx

@michael-ciniawsky
Copy link
Member

@ndreckshage

but why is that agreed? of course it shouldnt be the responsibility of this to actually inject styles server side - it can do that on client.

Yep that's the main point, should it be opiniated and work client only and another loader is used for SSR or not. SSR could be supported if no additional setup is required, not talking about (export) here but something like browser: true | false (pseudo)

@d3viant0ne @bebraw @ekulabuhov We should make a final decision (again)

rendering react applications (for example) server side is pretty common, and people use webpack for both the client & server version of that application. why not support that?

I fully agree here

however, this doesn't impact me anymore since there are other good options for ssr styles that i prefer over css modules, like styled-jsx

If you in general lost interest in this PR and it would be stalled anyways please confirm, bc in that case we need to close it anyways 😛 It's been a while since someone took action, I can that decision

@ndreckshage
Copy link
Author

@michael-ciniawsky re: lost interest. Im happy to contribute & update PR if other people have benefit. I just personally likely wouldn't use it even if implemented. But I'd love to at least see some SSR suggestions in readme, even if pointing to a different loader / solution / official explanation that SSR not supported.

@michael-ciniawsky
Copy link
Member

@ndreckshage kk let's wait and yep other solutions like isomorphic-style-loader will get mentioned in the docs if SSR (this PR 😛 ) is rejected

@ekulabuhov
Copy link
Member

I don't have much of opinion on this as I don't use SSR either. It just seems like all the required work is done by css-loader and I don't see a good reason to use style-loader in SSR setup at all.

On the other hand, I agree that we need to direct people to a proper solution. Maybe even mention it in the exception thrown?

@ndreckshage
Copy link
Author

@ekulabuhov potential reasons to use style loader with SSR:

  • critical styles instead of css request. since we can bubble up styles used by our components.
  • single page app simplicity for injecting additional styles.
  • only worrying about js resources not css as well.

@bebraw
Copy link
Contributor

bebraw commented Mar 15, 2017

Given the change seems so small, I would be ok if you test it. Maybe clarify the PR to point out you should use this feature with SSR so people who need the feature will find it better.

@joshwiens
Copy link
Member

@bebraw - Tobias appears? to have strong feelings in regards to this particular topic. Not that I am big on pestering him but in this case, it may be prudent to run this particular feature past him.

Small change or not, SSR is a potential can of worms.

@bebraw
Copy link
Contributor

bebraw commented Mar 15, 2017

@d3viant0ne Yeah, that's a good point. Accepting this could lead to more work. I'll poke. 👍

@michael-ciniawsky
Copy link
Member

@ndreckshage Changed my mind about SSR support for style-loader 👍 for supporting it, but let's wait for the Master of Disasters approval 😛 @bebraw You try to reach him ?

@michael-ciniawsky
Copy link
Member

also @ekulabuhov please share you thoughts once again as you where also not too convinced about SSR support in style-loader 🙃

@bebraw
Copy link
Contributor

bebraw commented Mar 15, 2017

@michael-ciniawsky Yeah. I'll try to get in touch so we get a decision and can move forward.

@joshwiens
Copy link
Member

joshwiens commented Mar 15, 2017

My gut feeling is gigantic can of worms which is fine if this is a feature that is needed and actually belongs in style-loader but i'm defaulting to no unless Tobias says yes.

Too many possible moving parts & framework specific issues for this feature to be approved by anyone but him honestly.

@sokra
Copy link
Member

sokra commented Mar 15, 2017

Loaders should do a single job. The only job of the style-loader is to add style to the DOM. This job doesn't make sense on SSR. So it's incorrect to use the style-loader on server-side.

The PR changes it's behavior to export content which is the return value of require(remainingRequest). So if you want this behavior you could just omit the style-loader in configuration (leaving only css-loader). But this is weird: You are changing the semantics of import "style.css" in your code (client-side: only sideeffect, server-side: only exports CSS).

I think it's a general accepted fact that the semantic of import "style.css" is: Sideeffect: add CSS rules to the DOM. As this sideeffect doesn't make sense while SSR the best configuration is to use the null-loader instead of style-loader!css-loader while SSR. SS-rendered HTML will rely on the fact that CSS is already available on client-side. As the client-code makes the CSS available this is true. You'll see a FOUC if you include the script after the SSR-HTML. Solve this with the ExtractTextWebpackPlugin and put the style-tag before the SSR-HTML.


Ok you are asking about serverside-rendering critical CSS. There is a solution for this too. Just for completeness: Use a different loader on client-side and server-side for CSS. Semantics: import { use } from "style.css", use need to call use() to make the CSS available in the DOM. Do this in every component of your app. On server-side the loaders runtime tracks the calls to use() and returns all rules "used". Example: let { value: html, style } = trackUse(() => renderPage()). (Hint: you may notice the usable mode in the style-loader readme. I don't know if the server-side part exists, but it's simple)


Why not use render the only the critical CSS, HTML and JS in the entry point and load the remaining stuff on demand? Create a client-side entry point for every possible entry of your application (every SSR-page) and have a specialized stylesheet for this (extracted). This approach requires less code in your components and less JS on page load. Stylesheet is static (CDN-able). You can preload the remaining JS if it's important (-> link preload).

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.

9 participants