Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Enable CSS Modules by default in *.module.css files #495

Merged
merged 7 commits into from
Dec 1, 2017
Merged

Enable CSS Modules by default in *.module.css files #495

merged 7 commits into from
Dec 1, 2017

Conversation

eliperelman
Copy link
Member

Fixes #440.

@timkelty would you be interested in having a look at this?

@eliperelman eliperelman requested a review from a team November 29, 2017 15:38
@timkelty
Copy link
Contributor

@eliperelman will do!

@timkelty
Copy link
Contributor

timkelty commented Nov 30, 2017

@eliperelman not sure what I'm missing…but can't get your PR working:

As soon as I add a .module.css import, I pukes:

$ neutrino build
✖ Building project failed
./src/test.module.css
Module build failed: ModuleBuildError: Module build failed: Unknown word (3:7)

1 |
2 | if(module.hot) {

3 | // 1512017012399
| ^
4 | var cssReload = require("../../neutrino-dev/node_modules/css-hot-loader/hotModuleReplacement.js")(module.id, {"fileMap":"{fileName}"});
5 | module.hot.dispose(cssReload);
6 | module.hot.accept(undefined, cssReload);

at runLoaders (/usr/local/lib/node_modules/neutrino/node_modules/webpack/lib/NormalModule.js:195:19)
at /usr/local/lib/node_modules/neutrino/node_modules/loader-runner/lib/LoaderRunner.js:364:11
at /usr/local/lib/node_modules/neutrino/node_modules/loader-runner/lib/LoaderRunner.js:230:18
at context.callback (/usr/local/lib/node_modules/neutrino/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
at Object.<anonymous> (/Users/timkelty/Repos/neutrino-dev/node_modules/css-loader/lib/loader.js:50:18)
at /Users/timkelty/Repos/neutrino-dev/node_modules/css-loader/lib/processCss.js:234:4
at <anonymous>
at runMicrotasksCallback (internal/process/next_tick.js:121:5)
at _combinedTickCallback (internal/process/next_tick.js:131:7)
at process._tickCallback (internal/process/next_tick.js:180:9)

@ ./src/test.module.css
@ ./src/index.js
@ multi ./src/index
./src/test.module.css
Module build failed: ModuleBuildError: Module build failed: Unknown word (3:7)

Also tried disabling hot/extract just to debug, but got a similar error.

.neutrinorc.js

module.exports = neutrino => {
  neutrino.use('@neutrinojs/web', {
    hot: false,
    style: {
      extract: false
    }
  });
}

src/index.js

import css from './index.css';
import cssModule from './test.module.css';

console.log(css);
console.log(cssModule);

If I remove import cssModule from './test.module.css', it works fine, the rest build works.

Copy link
Contributor

@timkelty timkelty left a comment

Choose a reason for hiding this comment

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

Shouldn't we be setting options.css.modules = options.modules to actually enable modules in css-loader? https://github.com/webpack-contrib/css-loader#options

@eliperelman
Copy link
Member Author

lol yup. I'll revise.

@eliperelman
Copy link
Member Author

@timkelty OK, should be fixed.

@@ -3,63 +3,92 @@ const merge = require('deepmerge');

module.exports = (neutrino, opts = {}) => {
const options = merge({
test: opts.modules !== false
? /(?<!\.module)\.css$/
Copy link
Contributor

Choose a reason for hiding this comment

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

What version of node are you testing in?
It would seem negative lookbehinds don't work in node 8.x (they do in 9.x)

Copy link
Member Author

Choose a reason for hiding this comment

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

omg 🤣

OK, lemme get that fixed.

@eliperelman
Copy link
Member Author

@timkelty OK, give that a shot.

const isCssModule = cssModulesTest.test(input);
const isRegularCss = cssTest.test(input);

if (opts.modules !== false && isCssModule) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since opt.modules is always just a bool, why not just do opts.modules && isCssModule?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, for some reason I had it in my head that it could be an object, but I guess that's not the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I know why: opts.modules could be undefined, and in that case, it should be treated the same as true, since that would be the default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha.

Copy link
Contributor

@timkelty timkelty left a comment

Choose a reason for hiding this comment

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

LGTM!

See note about potentially setting ignoreOrder. Could go either way on it - just an idea.

fallback: {
loader: styleEntries[options.styleUseId].get('loader'),
options: styleEntries[options.styleUseId].get('options')
if (options.modules) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something to consider here might be to set options.extract.plugin.ignoreOrder: options.modules, just as a nice default: https://github.com/webpack-contrib/extract-text-webpack-plugin#options

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooo nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm - ignoreOrder is an option for options.extract.plugin, not options.css.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yikes, sorry. :(

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 we should be able to just do it up in the initial options merge, eg

    extract: {
      plugin: {
        filename: neutrino.options.command === 'build' ? '[name].[contenthash].css' : '[name].css',
        ignoreOrder: opts.modules !== false
      }
    }

@eliperelman
Copy link
Member Author

Thanks @timkelty for the great review!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants