-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
Conversation
Before, we had to pass a string to "Parser" option.
Using Spike with latest version of postcss-loader generates an error during compilation because parser is passed as an object. Error is :
Is it possible to use postcss-loader with parser as an object ? Thanks team. |
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. |
{Object}
(options.parser
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liitfr Thx
There was a problem hiding this 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.
Would be great to get a patch release for this when you have a moment! |
No description provided.