Support resolving Stylelint using PnP

[adalinesimonian]

Yarn 2.x and higher introduces PnP support, which is enabled by default. However, this extension does not support PnP, which is why Yarn's SDK tooling patches in a custom stylelintPath to allow Stylelint installed using PnP to be resolved. It would be great if we could have PnP work out-of-the-box without any additional configuration.

@arcanis I'd appreciate your input if you have the time to share what might be a good next step!

[arcanis]

Hey Adaline! In general the main thing that requires the SDK is the bootstrap. Since packages are stored within zip archives, and since the resolution for the package's dependencies (here stylelint) is controlled by Yarn, we need the runtime to know what to do when accessing the files or resolving dependencies. Within yarn run and yarn node commands it's done automatically (via NODE_OPTIONS), but IDEs run within their own runtime, which doesn't know how to do any of these things.

To address that, the files pointed by the SDK mostly just need to make sure that our loader is properly setup before shelling out to the "real" dependency. For instance, this is all the SDK file for ESLint does. First it requires the .pnp.cjs file, then it forwards to the real eslint/lib/api.js:

https://github.com/yarnpkg/berry/blob/master/.yarn/sdks/eslint/lib/api.js#L12-L20

So to get a "native" PnP support, you'd need to do the same thing by checking if there's a PnP file available (probably by checking the workspace root) and, if so, requiring it and calling its setup method before requiring the actual stylelint modules.

[adalinesimonian]

Thanks for the guidance! A few clarifying questions:

• We also currently read the Stylelint package manifest, which we use for things such as determining the package's version. require('/path/to/.pnp.cjs').setup() would allow us to require() the package, but if we want to pass a path to and then read the package manifest, I'm guessing we'd need to use some kind of zip library or something along those lines?
• We can't currently use ESM because of some limitations we have with working inside VS Code. With the new ESM support for PnP would we effectively be locked out of resolving Stylelint if the project is using ESM, since we'd have to read .pnp.mjs?
• Are we able to rely on .pnp.cjs or must we also include checks for .pnp.js? My understanding — correct me if I'm wrong — is that it used to be .pnp.js but got changed to .pnp.cjs, but Yarn still has a fallback for supporting .pnp.js. With ESM support, will it now always be named .pnp.mjs if it is in an ESM project?
• If we're working with multiple workspaces, and the language server runs setup() from a workspace's PnP file, will PnP resolution work in a different workspace with its own .pnp.cjs or will resolution only work in the workspace in which the PnP setup function was originally called?

[adalinesimonian]

I've pushed up a PR (#273) that introduces built-in support, pending our discussion to make sure we're doing what we need to be so that we do this right. The relevant lines that actually do the resolution are: https://github.com/stylelint/vscode-stylelint/pull/273/files#diff-8fd5fc572876748046180731f3d1a4539c274424e9f34b386d14d33585f602c2R61-R125

[arcanis]

• We also currently read the Stylelint package manifest, which we use for things such as determining the package's version. require('/path/to/.pnp.cjs').setup() would allow us to require() the package, but if we want to pass a path to and then read the package manifest, I'm guessing we'd need to use some kind of zip library or something along those lines?

Just calling our setup function from within the script that will perform the require should be enough - it'll inject the runtime within the process and the rest (like the zip accesses via fs) is transparent.

• We can't currently use ESM because of some limitations we have with working inside VS Code. With the new ESM support for PnP would we effectively be locked out of resolving Stylelint if the project is using ESM, since we'd have to read .pnp.mjs?
• Are we able to rely on .pnp.cjs or must we also include checks for .pnp.js? My understanding — correct me if I'm wrong — is that it used to be .pnp.js but got changed to .pnp.cjs, but Yarn still has a fallback for supporting .pnp.js. With ESM support, will it now always be named .pnp.mjs if it is in an ESM project?

It's likely we'll always generate the .pnp.cjs file, since most Node tools use it and I don't expect mjs to become the de facto standard for a very long time. As for .pnp.js, while it isn't used at all since Yarn 3.0, it's still used by 2.x users (~a third of Yarn 2+ users). If the cost of checking both isn't high I'd suggest to do it for an extra year, otherwise it's not a huge deal either.

• If we're working with multiple workspaces, and the language server runs setup() from a workspace's PnP file, will PnP resolution work in a different workspace with its own .pnp.cjs or will resolution only work in the workspace in which the PnP setup function was originally called?

It's a little unclear; VSCode Workspaces bring quite a bit of edge cases, and since we don't use them much in our team they aren't dogfooded as the rest of the integration. In theory you only need one runtime, no matter which one, which will then route the resolution to the proper resolution tables. In practice, the more complexity there is the higher is the chance for something unforeseen to happen...

[adalinesimonian]

Just calling our setup function from within the script that will perform the require should be enough - it'll inject the runtime within the process and the rest (like the zip accesses via fs) is transparent.

Fantastic! That makes everything so much easier.

It's likely we'll always generate the .pnp.cjs file, since most Node tools use it and I don't expect mjs to become the de facto standard for a very long time. As for .pnp.js, while it isn't used at all since Yarn 3.0, it's still used by 2.x users (~a third of Yarn 2+ users). If the cost of checking both isn't high I'd suggest to do it for an extra year, otherwise it's not a huge deal either.

I've added support for .pnp.js in #273 so that we can continue to support Yarn 2.x users until there's no longer a need.

It's a little unclear; VSCode Workspaces bring quite a bit of edge cases, and since we don't use them much in our team they aren't dogfooded as the rest of the integration. In theory you only need one runtime, no matter which one, which will then route the resolution to the proper resolution tables. In practice, the more complexity there is the higher is the chance for something unforeseen to happen...

Noted. Either way, sounds like our implementation wouldn't be any different in this regard from the editor SDK as the PnP runtime injection is still effectively happening in the same context, just in slightly different places. We'll just have to keep our eyes peeled in case we ever bump into anything weird. If that ever happens and it seems it may have more to do with Yarn or PnP than it does specifically with our extension, I'll loop you into it.

[adalinesimonian]

Built-in PnP support has been merged and will be released in 1.1!

[adalinesimonian]

PnP support released in v1.1.0!

Is there any follow-up we need to take a look at, e.g. updating Yarn docs?