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(index): copy loader options before modifying #326

Merged
merged 1 commit into from
Jan 3, 2018

Conversation

clydin
Copy link
Contributor

@clydin clydin commented Jan 3, 2018

As per the loader-utils documentation (here), the options object should not be modified directly. Doing so causes the loader parameter of a plugins option function to only be run once at the first use of the loader. This effectively causes the loader parameter to be useless as it cannot be used to retrieve information about the actual resource being used (i.e., loader.resourcePath, etc.) instead it will always point to the first resource the loader handled. This PR makes a copy of the options object returned from getOptions.

Type


  • Fix

SemVer


  • Bug (:label: Patch)

Issues


Checklist


  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.5%) to 87.333% when pulling 3d1f59a on clydin:clone-options into b9c1add on postcss:master.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@clydin Thx

@@ -51,7 +51,7 @@ module.exports = function loader (css, map) {

Promise.resolve().then(() => {
const length = Object.keys(options)
.filter((option) => {
.filter((option) => {
Copy link
Member

Choose a reason for hiding this comment

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

😅 👍

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jan 3, 2018

@clydin Did you test with postcss.config.js aswell ? This shouldn't be an issue here I assume...
Can you give an small usage example where this is reproducible ? (When exactly does this cause undesired behavior/breaks ?)

@michael-ciniawsky michael-ciniawsky changed the title fix(index): copy loader options before modifying fix(index): copy loader options before modifying Jan 3, 2018
@clydin
Copy link
Contributor Author

clydin commented Jan 3, 2018

The issue stems from this line which is modifying the options object: https://github.com/postcss/postcss-loader/blob/master/lib/options.js#L5

Since that line changes the plugins property value from the function to the returned array and that options object is still used and kept by webpack, the function will never be called again as the plugins property is now an array for the rest of the webpack compilation. This essentially captures the passed in loader context object inside the returned plugins array. So the example in the readme (https://github.com/postcss/postcss-loader#plugins) for accessing loader.resourcePath only works for the first resource loaded. Every subsequent file processed with the loader will have the same first loader.resourcePath.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jan 3, 2018

Released in v2.0.10 🎉 Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants