erikras / react-redux-universal-hot-example

A starter boilerplate for a universal webapp using express, react, redux, webpack, and react-transform
MIT License
12k stars 2.5k forks source link

CSS Module composes with different file #184

Open jepezi opened 8 years ago

jepezi commented 8 years ago

Hi,

Have you guys use composes feature of CSS Module? I have this error when I try to compose css class from different file.

/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack-isomorphic-tools/babel-transpiled-modules/plugin/write assets.js:209
[0]                 throw _iteratorError;
[0]                       ^
[0] SyntaxError: Unexpected token +
[0]     at Object.parse (native)
[0]     at module.exports.assets.style_modules.parser (/Users/jiratarinrith/codes/node/redux/erik/webpack/webpack-isomorphic-tools.js:46:42)
[0]     at /Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack-isomorphic-tools/babel-transpiled-modules/plugin/write assets.js:187:17
[0]     at Array.reduce (native)
[0]     at _loop (/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack-isomorphic-tools/babel-transpiled-modules/plugin/write assets.js:183:7)
[0]     at populate_assets (/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack-isomorphic-tools/babel-transpiled-modules/plugin/write assets.js:197:4)
[0]     at write_assets (/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack-isomorphic-tools/babel-transpiled-modules/plugin/write assets.js:59:2)
[0]     at Compiler.<anonymous> (/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack-isomorphic-tools/babel-transpiled-modules/plugin/plugin.js:161:32)
[0]     at Compiler.applyPlugins (/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack/node_modules/tapable/lib/Tapable.js:26:37)
[0]     at Watching._done (/Users/jiratarinrith/codes/node/redux/erik/node_modules/webpack/lib/Compiler.js:78:17)

Here's what I've tried.

const styles = require('./About.scss');
...
<h1 className={styles.heading}>About Us</h1>
...
/* About.scss */
.verybig {
  font-size: 60px;
}

.heading {
  color: red;
  composes: verybig;
}
.text-underline {
  text-decoration: underline;
}
/* About.scss */
.verybig {
  font-size: 60px;
}

.heading {
  color: red;
  composes: verybig;
  composes: text-underline from "./underline.css"; /* <---------------- add this */
}

This will give error from webpack-isomorphic-tools. I guess it has something to do with how composes work and the parser function.

Any idea how to fix this? @halt-hammerzeit @erikras

jepezi commented 8 years ago

@erikras Now that I think about it, this issue is not really about this example project. I can create new issue at webpack-isomorphic-tools repo if you want.

catamphetamine commented 8 years ago

Ok, I get it, the name I gave that file - webpack-isomorphic-tools.js - confuses everyone who has an error there and those people then conclude that it's a bug in my project. Maybe you could rename that file to something like isomorphic.js or whatever, but that's up to you if you want to do that at all.

As for the bug, @jepezi, can you tell me, where does the error occur according to your stacktrace?

bdefore commented 8 years ago

@jepezi I've run into the same issue https://github.com/halt-hammerzeit/webpack-isomorphic-tools/issues/1

For now, I'm working around it by changing the parser function of style_modules in webpack-isomorphic-tools.js to following the same parser as that in images ... so parser: WebpackIsomorphicToolsPlugin.url_loader_parser instead of the function that's there. I've been meaning to investigate further. The error you're getting is because by the time the tools encounter what should be a string, it's css-loader adding concatenating the path with a require call to the other file you're composing. And the + operator makes that invalid JSON. Very well might be an issue in their logic.

catamphetamine commented 8 years ago

My thought is that it's not an issue in anyone's logic. It's just that that composes thing doesn't inline styles and resorts to require() instead. Therefore, there's only one solution I can think of: to get the source of the require()d module from the list of the modules and then manually replace + require(...) + with the contents of the module or whatever else. I can modify webpack-isomorphic-tools to pass a list of all the modules (and their contents) to the parser function. Do you want that? Will you leverage that? Are you willing to do the replacement?

jepezi commented 8 years ago

@halt-hammerzeit @bdefore Yeah I think you're right. Just tried url_loader_parser, I got this in webpack-stats.json to confirm that it works.

// exports part exports.locals = {\n\t\"verybig\": \"verybig29YVG\",\n\t\"heading\": \"heading2jEei verybig_29YVG \" + require(\"-!./../../../nodemodules/css-loader/index.js?modules&importLoaders=2&sourceMap&localIdentName=[local][hash:base64:5]!./../../../node_modules/autoprefixer-loader/index.js?browsers=last 2 version!./../../../node_modules/sass-loader/index.js?outputStyle=expanded&sourceMap!./text-underline.css\").locals[\"text-underline\"] + \"\"\n}

trueter commented 8 years ago

That would be great.

catamphetamine commented 8 years ago

so I made a small change to webpack-isomorphic-tools and now it exposes webpack_stats inside parser functions (options.webpack_stats). a possible way to parse the style would be to take the source:

// exports part
exports.locals = {\n\t\"verybig\": \"verybig___29YVG\",\n\t\"heading\": \"heading___2jEei 
verybig___29YVG \" + require(\"-!./../../../node_modules/css-loader/index.js?
modules&importLoaders=2&sourceMap&localIdentName=
[local]___[hash:base64:5]!./../../../node_modules/autoprefixer-loader/index.js?browsers=last 2 
version!./../../../node_modules/sass-loader/index.js?outputStyle=expanded&sourceMap!./text-
underline.css\").locals[\"text-underline\"] + \"\"\n}

Take the JSON part

{\n\t\"verybig\": \"verybig___29YVG\",\n\t\"heading\": \"heading___2jEei verybig___29YVG \" + require(\"-
!./../../../node_modules/css-loader/index.js?modules&importLoaders=2&sourceMap&localIdentName=
[local]___[hash:base64:5]!./../../../node_modules/autoprefixer-loader/index.js?browsers=last 2 
version!./../../../node_modules/sass-loader/index.js?outputStyle=expanded&sourceMap!./text-
underline.css\").locals[\"text-underline\"] + \"\"\n}

And replace all such things:

\" + require(\"-!./../../../node_modules/css-loader/index.js?
modules&importLoaders=2&sourceMap&localIdentName=
[local]___[hash:base64:5]!./../../../node_modules/autoprefixer-loader/index.js?browsers=last 2 
version!./../../../node_modules/sass-loader/index.js?outputStyle=expanded&sourceMap!./text-
underline.css\").locals[\"text-underline\"] + \"

with something like

required = find ./text-underline.css from options.webpack_stats.modules
required = required.source
replaced = take the css loaders array from webpack.config.js and transform it to a string prepending "./../../../node_modules/"
replace(replaced with required)
catamphetamine commented 8 years ago

Still, the possible way I suggested is a very hacky one and I don't think anyone would go that way. The other way is to introduce and option in css-loader (?) to inline modules instead of requiring them.

bdefore commented 8 years ago

thanks for doing that nikolay... I'll explore this further shortly. I have a hunch like yours that having the css-loader handle composing better is the right long term option

On Monday, September 14, 2015, Nikolay notifications@github.com wrote:

Still, the possible way I suggested is a very hacky one and I don't think anyone would go that way. The other way is to introduce and option in css-loader (?) to inline modules instead of requiring them.

— Reply to this email directly or view it on GitHub https://github.com/erikras/react-redux-universal-hot-example/issues/184#issuecomment-140045846 .

fingermark commented 8 years ago

I'm still getting this error. What's the solution?

fingermark commented 8 years ago

Also found this: https://github.com/css-modules/css-modules/issues/13

bdefore commented 8 years ago

@fingermark i'd love to hear if you've solved this.

@halt-hammerzeit i explored your situation but in the end felt stuck on how i'd get a proper class name out of the step you describe:

replaced = take the css loaders array from webpack.config.js and transform it to a string prepending "./../../../node_modules/"

Is the objective to end up with a mapping to something like heading___2jEei verybig___29YVG my-composed-class___29YVG ?

catamphetamine commented 8 years ago

@bdefore the objective is to end up with whatever you are aiming for. I'm not using css modules myself.

I guess what you are aiming for is a JSON object which maps something to something. You can get some clues looking into the corresponding JSON object in the original boilerplate.

fingermark commented 8 years ago

@bdefore I have not. I gave up on a universal app for the time being. I was able to compose just fine with react-starter-kit, though, but had issues with changes permanently going into effect with BrowserSync. I'm using my own starter-kit now w/o universal support and everything is working just fine.

jepezi commented 8 years ago

Hi it's been a while. What I end up doing was to completely change the way to set up webpack config and run it on server code as well, then all require calls on server are "webpacked".

bdefore commented 8 years ago

@jepezi not sure i understand. could you provide that config?

shark404 commented 8 years ago

Hi, so I've hacked together a solution based on what halt-hammerzeit mentioned above. I'm completely new to React and Webpack and I've made a couple of assumptions about how things work so this may not be a 100% solution but it seems to work in my limited testing. I've hacked it together quickly and it's late for me so please forgive me if it's a bit messy and not so elegant...

I had an issue with the paths in the require call being different to what was available in the webpack_stats.modules. e.g. require(!-./../../../node_modules/css-loader/index.js?modules instead of ./~/css-loader?modules so I'm just checking against last part of the module name i.e. the path and filename for the css file.

This lead to a second problem where that actually matches multiple modules in the webpack_stats.modules. So I'm taking the first one because that seems to be the one with the correct source. Big assumption here...

@bdefore this is my new parser config in webpack-isomorphic-tools.js

      parser: function(m, options, log) {
        if (m.source) {
          log.info("source of " + m.name);
          var regex = options.development ? /exports\.locals = ((.|\n)+);/ : /module\.exports = ((.|\n)+);/;
          var match = m.source.match(regex);
          if (match) {
            var requireRegex = /" \+ require\("-!(?:[^)]*\!)+([^)]*)"\)\.(?:locals|exports)\["([^\]]*)"\] \+ "/

            var result = match[1];
            var requireMatch;
            while (requireMatch = result.match(requireRegex)) {
              // require found. lookup from other modules
              var requireModule = options.webpack_stats.modules.filter(function(el) {
                return el.name.endsWith(requireMatch[1]);
              });
              var requireSource = requireModule[0].source.match(regex);
              var requireOutput = JSON.parse(requireSource[1])[requireMatch[2]];
              result = result.replace(requireMatch[0], requireOutput)
            }
            return JSON.parse(result);
          } else {
            return {};
          }
        }
      }
catamphetamine commented 8 years ago

@shark404 oh, so you actually made it. seems that you really like your .scss that you've gone so far.

As for the relative ../../../../... paths, that's yet another uneasy task... (if it needs to be solved at all) Here are some of my thoughts on this. A lot of hackery below. The process is tricky and some "assumptions" are made too. In case anyone really wants these possible steps to be implemented:

catamphetamine commented 8 years ago

(i've updated my post above: my previous code was irrelevant and this seems to me a significantly more complex task than I've estimated in the beginning)

shark404 commented 8 years ago

@halt-hammerzeit I just really like the idea of modular css as it allows me to do something like

In stylesheets/components/btn.css:

.btn {
  // base styles for btn
}
.primary {
  composes: btn;
  // styles for a primary btn
}

Then in a specific component I may want to override something from the .btnPrimary. In MyForm.css

.submit {
  composes: primary from "stylesheets/components/btn.css"
  // extra styling for the submit btn
}

Then in my React component I can just require MyForm.css and style the submit button with the .submit class. I can achieve the same thing without composes by requiring btn.css as well and applying multiple classes, but then I need to worry about selector priority and it gets messy.

Maybe I'm just not doing things in a "React" way and there is a better way to approach this? If anyone has any insights I'd love to hear.

catamphetamine commented 8 years ago

@shark404 oh, I see. That's a handy feature. I guess, as a simpler solution, I would try to use mixins: http://sass-lang.com/guide There's a section called "Mixins" There's also a section called "Inheritance" below "Mixins"

shark404 commented 8 years ago

@halt-hammerzeit the issue with sass Mixins and Inheritance combined with Webpack is that the end result is a lot of duplicated css. Maybe I'm missing something but I believe it works l like this:

base.scss:

.baseStyle: {
  // base styles
}

FirstComponent.scss:

@import "base";

.firstStyle: {
  @extend .baseStyle;
  // extra styles
}

When I require('./FirstComponent.scss') the sass-loader is first to run, then the css-loader. The output css would look something like:

.baseStyle_hash1111, .firstStyle_hash2222 {
  // base styles
}
.firstStyle_hash2222 {
  // extra styles
}

Now if I have a second component that needs baseStyle and I do the same as with FirstComponent, when I require('./SecondComponent.scss') the output would be something like:

.baseStyle_hash3333, .secondStyle_hash4444 {
  // base styles
}
.secondStyle_hash4444 {
  // extra styles
}

The class name hashes will be different because that's how it's supposed to work. Everything is supposed to be local to the require. So we are left with the base styles being duplicated every time you want to extend them.

Maybe having the duplicated css isn't much of an issue and I'm just being stubborn, but css modules seem like the most elegant way to achieve this. As I mentioned I'm completely new to React and Webpack so maybe there is a better solution to what I'm trying to achieve, or maybe I should be thinking in the React way and I shouldn't be trying to share css like that?

For the moment, my hack solution is good enough for my needs :smile:

catamphetamine commented 8 years ago

@shark404 you are right, the CSS is duplicated in the case of mixins. i'm not sure if HTTP gzip eliminates the bulk.

you're right that in the programming world there's no absolute solution and one should use what's good enough.

bdefore commented 8 years ago

@shark404 thanks for posting that. i'll have a go at integrating your parser function and follow up in the next few days.

catamphetamine commented 8 years ago

Yo, everybody. So I've been hacking this whole webpack modules thing these days and came up with a better solution. The new version of webpack-isomorphic-tools (v2) now can eval() all that hacky syntax so all your "composes" things should work out of the box (it evals all the aforementioned "..." + require("../../...") + "..." stuff, etc). I've made a PR for the new version of webpack-isomorphic-tools: https://github.com/erikras/react-redux-universal-hot-example/pull/463 I've tested it a bit in development and production modes and it seems to work. This new version can also eliminate the initial flicker (flash) on page load in dev mode (caused by style-loader not yet loaded its styles). I have eliminated the flash in my project, but in this project I didn't do that because I didn't see a way to know (in Html.js) which style (App.scss, or Home.scss, or else) should it apply (didn't see a way to extract that info from the component variable).

catamphetamine commented 8 years ago

In the meantime, while my PR is pending, I've just released v2.1.0 of webpack-isomorphic-tools and I've also written a small test case with the composes feature and it works now:

:local(.black2)
{
    background: black;
}

:local(.blacker)
{
    composes: black2;
    composes: black from "./test.scss";
}
catamphetamine commented 8 years ago

The PR has been merged. I've also released v 2.2.0 which doesn't break anything but is better. And in 2.2.0 composes works too.

(in 2.2.0 webpack-assets.json is prettier)

quicksnap commented 8 years ago

Any feedback on if the new isomorphic tools resolves this issue?

catamphetamine commented 8 years ago

@quicksnap Well, how is my message, which is an inch higher than yours, not a feedback?

shark404 commented 8 years ago

@quicksnap I've updated and after limited testing it seems to be working fine. Thanks @halt-hammerzeit ! :smile: