thgh / rollup-plugin-scss

Rollup and compile multiple .scss, .sass and .css imports
MIT License
134 stars 47 forks source link

Added Yarn PnP support #83

Closed eagerestwolf closed 3 years ago

eagerestwolf commented 3 years ago

In connection with issue #60, this pull request adds Yarn PnP support by allowing Yarn to provide any of the necessary files from NPM. All local files are still simply passed directly to the sass compiler of the user's choice.

eagerestwolf commented 3 years ago

Ok, it's finally all working. You can now import a file in the following ways:

My importer has a specific case to handle local files. If an import begins with ., then the importer just immediately returns null and allows sass to handle the import. This should also improve performance because the plugin is now only trying to resolve non-local files. There is still one more edge case that I have not tested that would probably be an issue...url imports. My importer is going to attempt to resolve those with Node/Yarn PnP, which will probably fail. At this time, I am thinking of how to handle that.

Finally, thanks to @merceyz for the tip about letting Yarn PnP know what is actually requesting the file, this patch doesn't require pnpMode: loose or any packageOverrides in .yarnrc.yml. Side note, I only just realized that he/she is actually a Yarn developer, so thanks for coming to my aid, the documentation on making existing packages work with PnP is kinda horrible. As soon as I come up with a way to detect and reject urls with will be ready to merge. Additionally, once I come up with a way to detect and reject urls, I will add a check to the end of the resolution to ensure the file actually exists. That way, in the event a file doesn't actually exist, sass can go about handling the import and it will throw the error.

eagerestwolf commented 3 years ago

Ok, final code is done. Per the sass spec, url imports must start with http, https, or url, so I added checks for http and url in my bail code. This should be the final commit and everything is working on the local end. I guess I could test a url import right quick to be sure, but I think everything is good.

eagerestwolf commented 3 years ago

I also added some comments, so everyone can see what and why the code is doing what it does.

eagerestwolf commented 3 years ago

Can confirm, url imports are working as intended. As far as I can tell, everything is good and the green light can be given. Unless anyone can think of an edge case I may have missed.

eagerestwolf commented 3 years ago

I updated the code comments to be a bit more accurate now that I have a better understanding of all of this. I also surrounded the require.resolve() call with its own try/catch block, so I could defer processing back to sass. This is done in the event that a user did a relative import but didn't include the leading . because sass supports that, so it can still handle it.