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

Fix break: use regexp to detect css-loader #68

Closed
wants to merge 1 commit into from
Closed

Fix break: use regexp to detect css-loader #68

wants to merge 1 commit into from

Conversation

kouhin
Copy link

@kouhin kouhin commented Apr 14, 2016

v2.2.45 breaks my build absolutely!

I use the following code to check out the differences between v2.2.44 and v2.2.45,

const test1 = regex.test(module.name) && module.name.split('!')[0].indexOf('/~/css-loader') >= 0;
const test2 = WebpackIsomorphicToolsPlugin.style_loader_filter(module, regex, options, log);
if (test1 && !test2) {
  console.info('!!!!!!!!!!!!!Cannot MATCH', module.name);
}

Output:

!!!!!!!!!!!!!Cannot MATCH ../~/css-loader?modules&sourceMap&importLoaders=1&localIdentName=[local]___[hash:base64:5]!../~/postcss-loader?sourcemap!./styles/index.css
!!!!!!!!!!!!!Cannot MATCH ../~/css-loader?modules&sourceMap&importLoaders=1&localIdentName=[local]___[hash:base64:5]!../~/postcss-loader?sourcemap!./components/AppButton/index.css

So I create this PR to fix this issue.
It use regular expression to find the first loader, when the first loader is css-loader, true will be returned.
@halt-hammerzeit

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.819% when pulling 7b83959 on kouhin:feature/fix_style_loader_filter into 2d90c75 on halt-hammerzeit:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.819% when pulling 0e9f64e on kouhin:feature/fix_style_loader_filter into 2d90c75 on halt-hammerzeit:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.819% when pulling fb17821 on kouhin:feature/fix_style_loader_filter into 2d90c75 on halt-hammerzeit:master.

@catamphetamine
Copy link
Owner

@kouhin Hello.
I guess that might be because of the yesterday's pull request
#67

@catamphetamine
Copy link
Owner

Actually I knew this may happen, but wasn't sure.

Let's think about why does css-loader begin with ../~/css-loader and not ./~/css-loader.
You seem to be running webpack from a folder, and this folder doesn't contain node_modules/css-loader inside.
Can you tell more about your folder structure and why is that so?

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

@halt-hammerzeit Yes, you are right.
I put the webpack.config.babel.js and isomorphic.config.js in the /src/webpack/ folder

@catamphetamine
Copy link
Owner

From which folder are you running Webpack?
And which folder contains all the node_modules?

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

This is my folder structure

  • /
    • package.json
    • /node_modules
    • /src
      • /webpack
        • webpack.config.babel.js
        • isomorphic.config.js

I run webpack from the root of the project. Actually, I wrote a node script in package.json.

// package.json
{
  "scripts": {
    "build:client": "webpack ./src/webpack/webpack.config.babel.js"
  }
}

and run

npm run build:client

@catamphetamine
Copy link
Owner

I don't see why does webpack take the css loader from the parent folder (../~/css-loader) and not from the current folder (./~/css-loader).
The reason must be found first.
What is your context in webpack.conf.babel.js?
Is it equal to the root folder?

@catamphetamine
Copy link
Owner

I'm out for lunch, will be online in an hour

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

Right!

This is my config:

// in webpack.config.babel.js
module.exports = {
  devtool: DEV ? 'source-map' : false,
  context: path.resolve(__dirname, '..'),
  entry: {
    main: './client',
  },
...

The context is path.resolve(__dirname, '..'), maybe this is the reason.
I'd like to change the context and confirm it.

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

Confirmed!

After I removed the context from webpack config, and changed entry to entry: ./src/client the module.name was changed to be

./~/css-loader?modules&sourceMap&importLoaders=1&localIdentName=[local]___[hash:base64:5]!./~/postcss-loader?sourcemap!./src/styles/index.css

And then I tried to change the context to context: path.resolve(__dirname, '.'), and entry to entry: ../client, the module.name is

/Users/kouhin/git/front_project/~/css-loader?modules&sourceMap&importLoaders=1&localIdentName=[local]___[hash:base64:5]!/Users/kouhin/git/front_project/~/postcss-loader?sourcemap!../styles/index.css

@catamphetamine
Copy link
Owner

When you do context: path.resolve(__dirname, '.') then __dirname is src/webpack so the context becomes src/webpack which is not what you want.
context: path.resolve(__dirname, '..') becomes src which is also not what you want.
I suppose context: path.resolve(__dirname, '../..') is what you'd like it to be.

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

No, the context can't be path.resolve(__dirname, '../..'), because when I do this, webpack-assets.json will be put into root.
The problem is that the string ./src will also be included in webpack-assets.json, for example:

// `/webpack-assets.json`
{
  "javascript": {
    "main": "http://localhost:3001/project/assets/sp/20160414-ad47ad8/main.js"
  },
  "styles": {},
  "assets": {
    "./src/components/Header/HeaderUserInfo.css": {
      "thumbProf": "thumbProf___3UeI_",
      "thumbImg": "thumbImg___AfLtz",
    }
  }
...

In the production environment, all the /src will be compiled to /target by babel for server side.
Then when I run the server code by node ./target/start-server.js, it will cause errors like:

[webpack-isomorphic-tools] [error] asset not found: ./target/components/Header/HeaderUserInfo.css

@catamphetamine
Copy link
Owner

the string ./src will also be included in webpack-assets.json

Yes

Currently there's no ready solution for target folder compiled from src folder.
I am myself not using js compilation step.

If you want to make the new version work as the old one you can try to set your context to context: path.resolve(__dirname, '..').
Will it work for you with the new version?

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

Thank you! I have already used this patch in my project and it works as expected.

@catamphetamine
Copy link
Owner

Ok, I'm closing this PR then.
You regexp solution is good enough but since currently there are only 3 valid values we'll leave it as it is for now.

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

@halt-hammerzeit
OK, but I think since the pnpm and cnpm/npminstall is not the standard way, it is no need to add these specific values.
We need a more generic way to detect the css-loader.

I will propose another solution later.

catamphetamine added a commit that referenced this pull request Apr 14, 2016
@catamphetamine
Copy link
Owner

@kouhin You said 2.2.45 works now for your project with the context set. Is it true?
I thought it wouldn't work since the path to the loader would still be ../~/css-loader and not ./~/css-loader.
Maybe you meant that 2.2.45 still doesn't work for your project?

Your solution would work for (anything)/css-loader. While this is not a highly probable case, it is still theoretically possible that a user has some kind of src/css-loader/Component.scss file which would be classified as a css-loader-loader file.
Still it's a valid solution for almost all cases.

I've modified the current solution a bit.
Try webpack-isomorphic-tools@2.2.46
Does it work better for you than 2.2.45?

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

You said 2.2.45 works now for your project with the context set. Is it true?

No, I just use a customized filter to make the build work.

Your solution would work for (anything)/css-loader. While this is not a highly probable case, it is still theoretically possible that a user has some kind of src/css-loader/Component.scss file which would be classified as a css-loader-loader file.

I have noticed this pattern, so I use /(?:\/)([^\/\?\.~]+\-loader)(?:[\?!@])/.
You might notice this part (?:[\?!@]), it restricts the matched string xxx-loader must be followed with a ?(for query string) or ! or @(for version string).

The problem of this solution, is that the regexp can find all loaders within the module.name. It's not necessary and resulted in a bad performance.

Maybe this would be better.

Webpack_isomorphic_tools_plugin.style_loader_filter = function(module, regular_expression, options, log)
{
  const css_loader = module.name.split('!')[0]
  return /\/css-loader[\?!@]/.test(css_loader);
}

@kouhin
Copy link
Author

kouhin commented Apr 14, 2016

Thank you for the new version, I will try it tomorrow.

@CrocoDillon
Copy link

CrocoDillon commented May 11, 2016

@kouhin can you explain how you made this work? I also like to precompile my code, in my case from src to lib folder.

What did you put as context and entry in the Webpack config and what did you pass as basepath to webpack-isomorphic-toolsserver method? How did you copy the assets to the build folder? Anything else I’m missing? I can’t make it work.

catamphetamine added a commit that referenced this pull request May 11, 2016
@kouhin
Copy link
Author

kouhin commented May 12, 2016

@CrocoDillon

This is my folder structure:

  • [Project Root]
    • src/
      • client.js // Client-side entry
      • server.js // Server-side entry
      • webpack/
        • isomorphic.config.js // webpack-isomorphic-tools config
        • webpack.config.babel.js // webpack config
    • target/

This is partial config in webpack.config.babel.js

module.exports = {
  context: path.resolve(__dirname, '..'), // Make context root to /src
  entry: {
    main: './client', // This is /src/client.js
  },
  output: {
    path: path.resolve(__dirname, '..', '..', 'build', version),
    publicPath: `${CDN_PATH}/${version}/`,
    filename: '[name].js',
    chunkFilename: '[id].js',
  },
  ...
  plugins: [
    //... other plugins
    webpackIsomorphicToolsPlugin.development(DEV),
  ]

Actually, in isomorphic.config.js, I am using a customized filter for css files,

    style_modules: {
      extensions: ['css'],
      filter: function styleFilter(module, regex, options) {
        if (options.development) {
          const loader = module.name.split('!')[0];
          return /\/css-loader[\?!@]/.test(loader);
        }
        return regex.test(module.name);
      },
      path: function stylePath(module, options, log) {
        if (options.development) {
          return WebpackIsomorphicToolsPlugin.style_loader_path_extractor(module, options, log);
        }
        return module.name;
      },
      parser: function styleParser(module, options, log) {
        if (options.development) {
          return WebpackIsomorphicToolsPlugin.css_modules_loader_parser(module, options, log);
        }
        return module.source;
      },
    },

Build step:

  1. run webpack --config ./src/webpack/webpack.config.babel.js, all the client side files such as js, img, css will be put into /build folder, and webpack-assets.json will be put into /src folder.
  2. run babel --copy-files -d target src, then all the js files will be transformed & copied from /src to /target. Use --copy-files to copy non-js files such as webpack-assets.json
  3. deploy /build to CDN.
  4. run node ./target/server.js to start server.

Hope this helps!

@CrocoDillon
Copy link

Awesome! Thanks for the very elaborated response 😄 That’s definitely helpful, I can’t wait to try this later today!

catamphetamine pushed a commit that referenced this pull request May 13, 2016
@catamphetamine
Copy link
Owner

FYI
I'm posting this in every issue and PR to notify whoever may be interested:
today I've released an alternative helper library called universal-webpack.
It takes a different approach than webpack-ismorphic-tools and instead of hacking Node.js require() calls it just compiles all code with target: 'node' webpack configuration option.
As a result, all Webpack plugins and features are supported.
If you think you might need that here's an example project:
https://github.com/halt-hammerzeit/webpack-react-redux-isomorphic-render-example

@mmahalwy
Copy link

@kouhin running into this problem too. I cannot get the paths in webpack-assets.json to change from ./src to (for me) ./dist

Thoughts?

@catamphetamine
Copy link
Owner

@mmahalwy So have you tried copying assets from src to dist?

@CrocoDillon
Copy link

@mmahalwy, using @kouhin's instructions I got it to work here: CrocoDillon/universal-react-redux-boilerplate. Maybe that helps you as well.

@mmahalwy
Copy link

@halt-hammerzeit yes, all assets are copied over. The problem is that my webpack-assets.json still references ./src/PATH/TO/ASSETS

@catamphetamine
Copy link
Owner

catamphetamine commented Jul 22, 2016

@mmahalwy If you didn't get it, what HOU Bin proposed is to set Webpack context not to the root project folder but instead to src folder. That's not considered as a best practice but in this particular scenario it might work.

module.exports = {
  context: path.resolve(__dirname, '..'), // Make context root to /src
}

The fact that all assets in your webpack-assets.json point to ./src folder means that your Webpack context still points to the project root folder and doesn't point to ./src.

@mmahalwy
Copy link

mmahalwy commented Jul 22, 2016

Yes, and once i change the context to ./src webpack fails. Let me try again.

But regardless, I want them to point to ./dist and not ./src

@catamphetamine
Copy link
Owner

@mmahalwy Define "fails"

@mmahalwy
Copy link

Okay, I got it to work. It was 'failing' or not compiling because of the entry files still had reference to ./src

Thanks alot folks!

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

5 participants