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: allow to pass an {Object} (options.parser) #257

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

liitfr
Copy link
Contributor

@liitfr liitfr commented Jun 13, 2017

No description provided.

Before, we had to pass a string to "Parser" option.
@liitfr
Copy link
Contributor Author

liitfr commented Jun 13, 2017

Using Spike with latest version of postcss-loader generates an error during compilation because parser is passed as an object. Error is :

✗ ERROR
{ ModuleBuildError: Module build failed: Validation Error

PostCSS Loader Invalid Options

options.parser should be string

    at runLoaders (/home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/webpack/lib/NormalModule.js:192:19)
    at /home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/loader-runner/lib/LoaderRunner.js:364:11
    at /home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/loader-runner/lib/LoaderRunner.js:230:18
    at runSyncOrAsync (/home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/loader-runner/lib/LoaderRunner.js:143:3)
    at iterateNormalLoaders (/home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/loader-runner/lib/LoaderRunner.js:229:2)
    at Array.<anonymous> (/home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/loader-runner/lib/LoaderRunner.js:202:4)
    at Storage.finished (/home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:38:15)
    at /home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:69:9
    at /home/mathias/.nvm/versions/node/v8.1.0/lib/node_modules/spike/node_modules/graceful-fs/graceful-fs.js:78:16
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:503:3) id: '208ed5bd' }

Is it possible to use postcss-loader with parser as an object ?

Thanks team.
Mathias

@jescalan
Copy link
Contributor

jescalan commented Jun 14, 2017

Extra context -- it's important to be able to pass the actual parser as required as opposed to passing a string and having this library require it for you, since you cannot guarantee that the require path of this library will match the context you are using it in (for global packages especially this is the case). The actual code can accommodate this, which is why the requires from a string are inside an if statement, however, the recently added validation broke this.

Thanks for fixing it here @liitfr! 🎉


Also as an aside, honestly I think it's an antipattern to have loaders require dependencies from a string for you. Require resolution is not as simple as this often times, and it's best to force a guarantee that you are requiring the correct library from the correct location by asking the user to pass the actual item, rather than a string. Using a string is limiting and has a lot of edge cases that can cause bugs and confusing behavior (require path must be exactly correct within this library, local requires will not necessarily work, etc), whereas the option to pass in a parser directly is the opposite. If any maintainers also agree here, I would be happy to add a PR that eliminates the ability to pass a string and have it required internally, although this would be a breaking change for the next semver major update.

@michael-ciniawsky michael-ciniawsky changed the title Allow to pass an object to "Parser" option fix: allow to pass an {Object} (options.parser) Jun 14, 2017
@michael-ciniawsky michael-ciniawsky self-assigned this Jun 14, 2017
@michael-ciniawsky michael-ciniawsky added this to the 2.0.6 milestone Jun 14, 2017
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.

@liitfr Thx

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

@michael-ciniawsky All good, but it's interesting when we can release a new major version with support only config files. I still have many bugs with this.

@michael-ciniawsky michael-ciniawsky merged commit 4974607 into webpack-contrib:master Jun 14, 2017
@jescalan
Copy link
Contributor

Would be great to get a patch release for this when you have a moment!

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.

4 participants