Skip to content

fix(webpack): fix css sourceMap option #3272

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

Closed
wants to merge 2 commits into from
Closed

fix(webpack): fix css sourceMap option #3272

wants to merge 2 commits into from

Conversation

mikejpeters
Copy link

CSS source maps were not correctly generated because sourceMap option is case sensitive.
Also several workarounds are necessary with LoaderOptionsPlugin.

Fixes #2020

mikejpeters and others added 2 commits November 24, 2016 12:43
CSS source maps were not correctly generated because `sourceMap` option is case sensitive.
Also several workarounds are necessary with LoaderOptionsPlugin.
postcss: [ autoprefixer() ]
postcss: [ autoprefixer() ],
// workaround for https://github.com/webpack/css-loader/issues/340
context: path.resolve(__dirname, './'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to use __dirname here?

Copy link
Author

Choose a reason for hiding this comment

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

It's the same value passed to context earlier in this file.

Choose a reason for hiding this comment

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

@hansl Solving this bug is quite important IMHO. Could you please take a closer look and propose an alternative if using __dirname is not acceptable?
I've tried the fix and it and works fine (at last).
Thanks a lot!

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative: project root.

The reason is the __dirname is the path to this file, but it might actually not be in the same directory than the project (for example, when you npm link the CLI). I think the correct solution is to use the project root.

Note that tutorial use __dirname because their webpack.config.js are at the root of their project. So in their case it's the valid thing to do.

Also, path.resolve(__dirname, '.') does nothing; just put the path in there.

Copy link
Author

@mikejpeters mikejpeters Dec 3, 2016

Choose a reason for hiding this comment

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

What I have done is just a simple workaround for the fact that loader options plugin makes the original webpack config inaccessible to the other plugins. Probably the most correct way to apply the workaround would be to assign the entire webpack config to a var, so that the values can be re-used like this:

var webpackConfig = {
  ...
  context: path.resolve(__dirname, './'),
  ...
};

webpackConfig.plugins.push(
  new webpack.LoaderOptionsPlugin({
    test: /\.(css|scss|sass|less|styl)$/,
    options: {
      postcss: [ autoprefixer() ],
      context: webpackConfig.context, // workaround
      output: {
        path: webpackConfig.output.path // workaround
      }
    }
  })
});

return webpackConfig;

I did not do this simply because I wanted to keep the changes I make in this merge request as simple as possible. That's why I copied the exact config values from higher up in this same file. If you want those values changed I think it should be a separate merge request.

See L41 and L44

]
],
// workaround for https://github.com/webpack/css-loader/issues/340
context: path.resolve(__dirname, './'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Author

Choose a reason for hiding this comment

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

See my other comment please

@hansl
Copy link
Contributor

hansl commented Dec 3, 2016

Is it possible to add an e2e test so we avoid regressoins in the future?

@filipesilva
Copy link
Contributor

Superseded by #3402, it includes the fixes found here and more work.

@filipesilva filipesilva closed this Dec 8, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source maps for css/sass
5 participants