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

CSS Module composes with different file #184

Open
jepezi opened this issue Sep 13, 2015 · 30 comments
Open

CSS Module composes with different file #184

jepezi opened this issue Sep 13, 2015 · 30 comments

Comments

@jepezi
Copy link
Contributor

jepezi commented Sep 13, 2015

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.

  • In About.js, add this require to the top of render() and add className to

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

.heading {
  color: red;
  composes: verybig;
}
  • Works great so far. Now let's add one more file, text-underline.css
.text-underline {
  text-decoration: underline;
}
  • Adjust .heading class in About.scss
/* 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
Copy link
Contributor Author

jepezi commented Sep 13, 2015

@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
Copy link
Contributor

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
Copy link
Collaborator

bdefore commented Sep 13, 2015

@jepezi I've run into the same issue catamphetamine/webpack-isomorphic-tools#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
Copy link
Contributor

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
Copy link
Contributor Author

jepezi commented Sep 13, 2015

@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": "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}

@trueter
Copy link
Contributor

trueter commented Sep 14, 2015

That would be great.

@catamphetamine
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

bdefore commented Sep 14, 2015

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
#184 (comment)
.

@fingermark
Copy link

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

@fingermark
Copy link

Also found this: css-modules/css-modules#13

@bdefore
Copy link
Collaborator

bdefore commented Oct 14, 2015

@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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor Author

jepezi commented Oct 14, 2015

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
Copy link
Collaborator

bdefore commented Oct 15, 2015

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

@shark404
Copy link

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
Copy link
Contributor

@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:

  • take that string from the 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\") call
  • strip the !- (an "assumption") from it; path will be ./../../../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
  • get the const prefix = path.substring(0, path.indexOf('node_modules')) + 'node_modules'; prefix will be ./../../../node_modules
  • var paths = path.split('!')
  • replace prefix with '!./~' in each element of paths
  • path = paths.join('!')
  • then you still have those -loader/index.js things, so there's even more non-trivial work there to be done.

@catamphetamine
Copy link
Contributor

(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
Copy link

@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
Copy link
Contributor

@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
Copy link

@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 😄

@catamphetamine
Copy link
Contributor

@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
Copy link
Collaborator

bdefore commented Oct 19, 2015

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

@catamphetamine
Copy link
Contributor

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:
#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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

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

@catamphetamine
Copy link
Contributor

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

@shark404
Copy link

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

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

No branches or pull requests

7 participants