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

Next #760

Closed
wants to merge 22 commits into from
Closed

Next #760

wants to merge 22 commits into from

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Aug 24, 2018

What kind of change does this PR introduce?
bugfix, feature, refactoring

Did you add tests for your changes?

Yes

If relevant, did you update the README?

Yep

Summary

Rewrite css-loader

Does this PR introduce a breaking change?

Yes

@alexander-akait
Copy link
Member Author

alexander-akait commented Aug 24, 2018

Looks node on appveyor generate other hashes when linux, very weird, maybe problem in https://github.com/css-modules/generic-names (we should update loader-utils to latest version, already require npm access)

@alexander-akait
Copy link
Member Author

what the hell with CI 😕

@michael-ciniawsky
Copy link
Member

Ideally we extract the needed interpolateName code from loader-utils and use it in generic-names directly or add a new method to one of the ICSS Utils and use that instead. The dependency on loader-utils is not needed

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 24, 2018

Hashes are not platform consistent to my knowledge, but maybe it was fixed in newer version of loader-utils

@alexander-akait
Copy link
Member Author

@michael-ciniawsky wip on updating

@alexander-akait
Copy link
Member Author

/cc @michael-ciniawsky CI green (problem in deps also fixed)

@joepie91
Copy link

Just gave this a quick testrun, and I ran into the following error:

ERROR in ./test.css
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
TypeError: Cannot read property 'foo' of undefined
    at Object.../3rdparty/css-loader-upstream/dist/cjs.js?!./test.css (/home/sven/projects/3rdparty/css-loader-upstream/dist/cjs.js??ref--5-1!/home/sven/projects/icss-loader-test/test.css:786:251)
    at __webpack_require__ (/home/sven/projects/3rdparty/css-loader-upstream/dist/cjs.js??ref--5-1!/home/sven/projects/icss-loader-test/test.css:684:30)
    at /home/sven/projects/3rdparty/css-loader-upstream/dist/cjs.js??ref--5-1!/home/sven/projects/icss-loader-test/test.css:751:93
    at Object.<anonymous> (/home/sven/projects/3rdparty/css-loader-upstream/dist/cjs.js??ref--5-1!/home/sven/projects/icss-loader-test/test.css:754:10)
    at Module._compile (/home/sven/projects/icss-loader-test/node_modules/v8-compile-cache/v8-compile-cache.js:178:30)
    at exec (/home/sven/projects/icss-loader-test/node_modules/mini-css-extract-plugin/dist/loader.js:55:10)
    at childCompiler.runAsChild (/home/sven/projects/icss-loader-test/node_modules/mini-css-extract-plugin/dist/loader.js:133:14)
    at compile (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compiler.js:296:11)
    at hooks.afterCompile.callAsync.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compiler.js:553:14)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:24:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/Hook.js:35:21)
    at compilation.seal.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compiler.js:550:30)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/Hook.js:35:21)
    at hooks.optimizeAssets.callAsync.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compilation.js:1294:35)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/Hook.js:35:21)
    at hooks.optimizeChunkAssets.callAsync.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compilation.js:1285:32)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/Hook.js:35:21)
    at hooks.additionalAssets.callAsync.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compilation.js:1280:36)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/Hook.js:35:21)
    at hooks.optimizeTree.callAsync.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compilation.js:1276:32)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/Hook.js:35:21)
    at Compilation.seal (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compilation.js:1213:27)
    at hooks.make.callAsync.err (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compiler.js:547:17)
    at _err0 (eval at create (/home/sven/projects/icss-loader-test/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:11:1)
    at _addModuleChain (/home/sven/projects/icss-loader-test/node_modules/webpack/lib/Compilation.js:1064:12)
 @ ./index.jsx 3:10-31

Full testcase can be found at https://git.cryto.net/joepie91/icss-loader-test. This compiled 'correctly' with css-loader 1.0.0 except for it producing the wrong class names due to #562, so presumably the CSS itself is correct.

I tested it against next by cloning the css-loader repository, switching to the next branch, npm installing it, and yarn linking it into my test repository. I didn't use npm link or install-from-git due to a litany of outstanding npm bugs. I've verified that the error really does occur in next and not in 1.0.0.

@joepie91
Copy link

On closer inspection, it also seems to not support CSS Modules? Or at least the composes syntax doesn't seem to do anything, and ends up as-is in my build product (when removing the :import block to avoid the compilation error).

There doesn't seem to be any option to enable support for this either, and the option for specifying a class name ident format has also disappeared in this release.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 27, 2018

For composes the loader likely needs to add it's own resolver, postcss-icss-composes is syntax only (to my knowledge)

@alexander-akait
Copy link
Member Author

alexander-akait commented Aug 28, 2018

@joepie91 old css modules doesn't work with next css modules (ICSS) in some cases, you can find example with css modules in README

@alexander-akait
Copy link
Member Author

@joepie91 also you have invalid code you try to import class, but no export :export{foo: foo}

@joepie91
Copy link

Hmm, the ICSS spec suggests that you should only specify exports for exported literals, not for class names?

An exportedValue does not need to be quoted, it is already treated as a literal string.

(Emphasis mine.)

How does that combine with having to explicitly export classes? Given that those are presumably not literal strings.

@alexander-akait
Copy link
Member Author

alexander-akait commented Aug 28, 2018

@joepie91 to avoid big size of locals you should export/import only what you need, css-loader@1 works same

@alexander-akait
Copy link
Member Author

All in master

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.

None yet

4 participants