Added Yarn PnP support #83

eagerestwolf · 2021-06-03T14:24:54Z

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.

index.ts

…resolved

…'t like sass's done function

eagerestwolf · 2021-06-05T09:24:00Z

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

importing a local sass file in JavaScript/TypeScript (in this case Rollup is actually handling the import):
```
import './styles/styles.scss';
```
importing a sass file from Node in JavaScript/TypeScript (again, in this case Rollup itself is handling the import):
```
import 'carbon-components/scss/globals/scss/styles.scss';
```

importing a stylesheet from Node in sass (this is handled by my custom importer that uses require.resolve() which works on standard node_modules projects and Yarn PnP):

@import '~carbon-components/scss/globals/scss/styles.scss';
// This will also work because the `~` character doesn't actually do anything
// I just included support for it because it's kind of a standard in the sass world
@import 'carbon-components/scss/globals/scss/styles.scss';

importing a local sass file in sass (sass is handling this case on it's own):
```
@import './components/Header/Header.scss';
```

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 · 2021-06-05T17:42:31Z

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 · 2021-06-05T17:43:14Z

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

eagerestwolf · 2021-06-05T17:48:40Z

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.

merceyz · 2021-06-05T18:36:43Z

index.ts

+                // Have Node resolve the path to the file we're requesting.
+                // When using a plain node_modules project, this works as
+                // expected. When using Yarn PnP, Yarn will unplug the package,
+                // and give the path to the file. Unplugging a file simply
+                // means unzipping it since most packages aren't equipped to
+                // work with zip archives directly. Also, the 'paths' option
+                // tells Yarn what file is requesting this file, so PnP
+                // integrity remains intact. For Node, paths serves as an
+                // additional set of paths to look for the import in.


Yarn doesn't unplug on demand, it's done on install if required due to native files, postinstalls, or when explicitly requested.
All packages that use the fs module can work with zip files as PnP patches it to support reading from them.

For Node, paths serves as an additional set of paths to look for the import in

For both PnP and Node it's to tell it where the resolution should start, so it's not "additional"
https://nodejs.org/dist/latest-v16.x/docs/api/modules.html#modules_require_resolve_request_options

Fair enough, that was a misunderstanding on my part. PnP is such a massive departure from what I'm used to that I am still coming to grips with it (not that that's a bad thing, this kinda needed to happen for a while). As for the fs module support is that due to Yarn patching it, or is that just native node?

Also, with regards to the resolution, that was really just overly technical documentation. The way it is phrased makes it sound as if it's adding new resolution paths, but I'll update the comment. Thanks for clearing that up.

Does this phrasing capture a bit more accurately what's going on?

// Have Node resolve the path to the file we're requesting. // When using a plain node_modules project, this works as // expected. When using Yarn PnP, Yarn will resolve an absolute // path to the file and the sass compiler will access that file // directly from the zip archive it's contained in. The 'paths' // option simply tells Node and Yarn where resolution should // start (i.e. who is requesting this file).

I mean technically, both Node and Yarn PnP are doing the same thing, so it's a bit misleading. They're both just getting the absolute path to a file. It's just in the case of Node, it's the runtime doing it and with Yarn PnP (and, as I've recently found out, pnpm), it's the package manager doing it.

Alternative verbiage, I'll let you decide which is more accurate and concise.

// Have Node or PnP resolve a path to the file we're // requesting. In the case of a standard node_modules project // Node will handle the resolution. For a PnP project, the // package manager will handle the resolution. Both will // return an absolute path to the file, which will be passed to // the sass compiler, so it can get the file in question. // // If a file cannot be located, this will generate an error.

I also don't specifically mention Yarn, since it isn't the only PnP based package manager.

Sorry for the late response, the way you ended up wording it looks great

As for the fs module support is that due to Yarn patching it, or is that just native node?

The PnP implementation patches it
https://github.com/yarnpkg/berry/blob/d2e592a74e1de6fcf9e2e97a5d9712cf7eba159c/packages/yarnpkg-pnp/sources/loader/_entryPoint.ts#L26-L37

merceyz · 2021-06-05T18:37:28Z

index.ts

+                // Finally, return the resolved file...if we can confirm it
+                // exists. If not, you guessed it...abort, and let sass handle
+                // the import
+                return existsSync(resolved) ? { file: resolved } : null


require.resolve will throw if it can't find it so this should always be true

I'm not so sure about that one. In my testing, when it couldn't find a file, it just assumed it was relative to the root of the plugin package. It never threw an exception, nor did Yarn (though I know Yarn patches this API for PnP). For example say I was looking for the file ./components/Header/header', if it wasn't able to resolve the path, it just assumed the path was {PROJECT_ROOT}/.yarn/cache/rollup-plugin-scss-https-ab37d281de-9cbfba33c6.zip/node_modules/rollup-plugin-scss/components/Header/header.scss'. Maybe that's a bug, or maybe it's intended behavior for relative imports, I'm not 100% certain on how the resolution works at the low level.

Correction, it is intended behavior. Turns out, the package resolve-patch does just that. If the path begins with a relative character, it just does an fs.resolve() and assume the path exists. Fair enough, since I've added in all the checks, you are correct, this shouldn't be needed.

Given what I now know (sorry, by the way, I'm not very familiar with the internals of Node or Yarn), do you think it's safe to just do return { file: resolved }? I mean, I passed local files and urls off to sass, and since require.resolve() should throw an error if a file can't be resolved, there's literally no reason the code would get that far if a file wasn't located.

Yeah, that should be fine

eagerestwolf · 2021-06-07T05:20:51Z

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.

thgh

Looks good to me, is it possible to add an automated test for this behaviour?

Added Yarn PnP support

2ff692a

eagerestwolf mentioned this pull request Jun 3, 2021

[Feature request ] Support pnp Yarn 2 resolution - "Error: Can't find stylesheet to import." #60

Open

Attempted fix for local resolution failing

2508d16

merceyz reviewed Jun 3, 2021

View reviewed changes

index.ts Outdated Show resolved Hide resolved

eagerestwolf added 6 commits June 4, 2021 01:23

Fixed local resolution. Attempted fix for Yarn PnP out of package access

37b176d

Fixed local resolution for real (just let libsass do it)

6d6904e

Actually check if the resolved file exists, not just that a file was …

154ae8b

…resolved

Final attempt to fix local resolution in sass files

1de854c

Testing, turns out I was testing things incorrectly. Also, yarn doesn…

759d980

…'t like sass's done function

Don't even attempt to resolve a file that begins with '.'

9df8c82

Added checks for urls and making sure the resolved file actually exists

aed5e2a

merceyz reviewed Jun 5, 2021

View reviewed changes

Surround require.resolve() with a try/catch block, updated comments

02b09b9

thgh mentioned this pull request Jun 29, 2021

Release v3 #68

Closed

15 tasks

thgh approved these changes Jun 29, 2021

View reviewed changes

thgh merged commit 9c91e1d into thgh:v3 Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Yarn PnP support #83

Added Yarn PnP support #83

eagerestwolf commented Jun 3, 2021

eagerestwolf commented Jun 5, 2021 •

edited

Loading

eagerestwolf commented Jun 5, 2021

eagerestwolf commented Jun 5, 2021

eagerestwolf commented Jun 5, 2021

merceyz Jun 5, 2021

eagerestwolf Jun 5, 2021

eagerestwolf Jun 5, 2021

eagerestwolf Jun 5, 2021 •

edited

Loading

eagerestwolf Jun 5, 2021

merceyz Jun 23, 2021

merceyz Jun 5, 2021

eagerestwolf Jun 5, 2021

eagerestwolf Jun 5, 2021

merceyz Jun 23, 2021

eagerestwolf commented Jun 7, 2021

thgh left a comment

Added Yarn PnP support #83

Added Yarn PnP support #83

Conversation

eagerestwolf commented Jun 3, 2021

eagerestwolf commented Jun 5, 2021 • edited Loading

eagerestwolf commented Jun 5, 2021

eagerestwolf commented Jun 5, 2021

eagerestwolf commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagerestwolf Jun 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagerestwolf commented Jun 7, 2021

thgh left a comment

Choose a reason for hiding this comment

eagerestwolf commented Jun 5, 2021 •

edited

Loading

eagerestwolf Jun 5, 2021 •

edited

Loading