search

emilgoldsmith / stylelint-custom-processor-loader

A Webpack loader for stylelint used with custom processors
MIT License
24 stars 3 forks source link

Fix for incorrect configPath resolution #65

Closed harrysolovay closed 5 years ago

harrysolovay commented 5 years ago

Fix for #64

emilgoldsmith commented 5 years ago

Thank you for being proactive and trying to fix the problem yourself @harrysolovay !

Could you please explain the rationale behind this code? I'm just afraid that while this may fix your problem it could break it for many others. I also think we still need to be using stringifyRequest at least in some shape or form. As you can see here https://www.npmjs.com/package/loader-utils#stringifyrequest. It is a while since I worked on this project though, and I'm not completely up to date with all the best practices of Webpack loaders

harrysolovay commented 5 years ago

Ahhh... I'm not sure what format is expected of lintArgument.configFile––but from what I can tell, it just wants a string path to the file. Maybe my assumption was wrong though (maybe loader-utils is doing something for your project that I'm overlooking? ). Otherwise, this should be as simple as trying to resolve the project root+configPath (or the node_modules+configPath in the case that a config package is being used). It's strange: I ran your tests and they all passed. Not sure why some CI tests failed. If you're able to look into it, I can answer any questions you might have about how Webpack loader practices differ today :) I'd love to get this PR ready so that I can use it in a project of mine. Really really cool tool you made 👍

emilgoldsmith commented 5 years ago

The real tests all pass, the failing is just because forks aren't given the environment variables needed to upload to Coveralls. So don't worry about that.

As for the reasoning for the stringifyRequest this is the explanation in the README of loader utils:

Why is this necessary? Since webpack calculates the hash before module paths are translated into module ids, we must avoid absolute paths to ensure consistent hashes across different compilations.

And require.resolve gives absolute paths right?

To be quite honest I don't have full understanding of all the reasons, and maybe it also isn't as important here as we're not using it in a require but just passing it to Stylelint?

I also don't quite remember what the purpose of this line was: https://github.com/emilgoldsmith/stylelint-custom-processor-loader/pull/65/files#diff-168726dbe96b3ce427e7fedce31bb0bcR60

Guess I should've commented better!

Speaking of commenting could you throw in some descriptive comments that describe why we need the try and catch and what the purpose of each of these logic branches is?

harrysolovay commented 5 years ago

Yep: require.resolve gives the absolute path––which I'm certain is preferable for Stylelint configuration. The try and catch was to prevent errors in the case that require.resolve gets used with a non-existent path... but I just moved it outside of that block and created a dedicated safeResolve function... I also added comments to the code itself 👍 manually resolving the path for Stylelint should work just fine (and also, the currently-live version is broken unless you pass null as the path option... aka. users can only define their config as .stylelintrc). So hopefully this does the trick. Can you please try it out?

harrysolovay commented 5 years ago

@emilgoldsmith I just tested it out. Fixed a typo & added it to this PR. It's ready to be merged & pushed 👍 please let me know if you have any issues with it. Also, I'm more than happy to help maintain this package :)

Kind regards,

Harry

harrysolovay commented 5 years ago

And yes! If it's null, then it defaults to .stylelintrc

emilgoldsmith commented 5 years ago

Awesome @harrysolovay ! I'll merge it now and publish it next time I'm home with my MFA device. And help with maintaining is always welcome!

emilgoldsmith commented 5 years ago

Published as 0.6.0! And also invited you as a collaborator :)

harrysolovay commented 5 years ago

@emilgoldsmith cheers!