Skip to content
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

[Bug] PnP API cannot be consumed if file located outside workspace #693

Closed
segrey opened this issue Jan 14, 2020 · 22 comments
Closed

[Bug] PnP API cannot be consumed if file located outside workspace #693

segrey opened this issue Jan 14, 2020 · 22 comments
Labels
bug Something isn't working

Comments

@segrey
Copy link
Contributor

segrey commented Jan 14, 2020

Describe the bug

require('pnpapi') fails if it's run in a file located outside Yarn workspace.
For example, IntelliJ consumes PnP API in this way (intellij-yarn-pnp-deps-tree-loader.js is located in IDE installation folder) and after updating Yarn to 2.0.0-rc.22 it fails. This disables IntelliJ Yarn 2 integration unfortunately.

To Reproduce

  1. Create an empty package.json (just {}).
  2. Run yarn policies set-version berry, it will install Yarn 2.0.0-rc.22
  3. Run yarn install
  4. Create api-client.js file with the following content
require('pnpapi');
console.log('OK');
  1. Running node --require ./.pnp.js api-client.js outputs OK
  2. Move api-client.js outside of Yarn workspace with mv api-client.js ..
  3. Running node --require ./.pnp.js ../api-client.js fails with Error: Cannot find module 'pnpapi'

Environment if relevant (please complete the following information):

  • OS: Linux
  • Node version 12.11.1
  • Yarn version 2.0.0-rc22
@segrey segrey added the bug Something isn't working label Jan 14, 2020
@segrey
Copy link
Contributor Author

segrey commented Jan 14, 2020

Related issue in IntelliJ: https://youtrack.jetbrains.com/issue/WEB-43298
@arcanis Is it possible to allow an external script to consume PnP API for workspace located at process.cwd()?

@arcanis
Copy link
Member

arcanis commented Jan 15, 2020

I think the regression occurred while I was working on the multi-tree improvement (#630). I think I'll fix that by making require('pnpapi') from a non-PnP module return an object with only the new findApiFromPath method (which I still need to add to the documentation). Would that work for you? You could use it like this:

const api = require(`pnpapi`).findApiPathFor(process.cwd());

@segrey
Copy link
Contributor Author

segrey commented Jan 15, 2020

@arcanis Sure, that will work.

@segrey
Copy link
Contributor Author

segrey commented Jan 15, 2020

@arcanis Could you please clarify these off-topic questions:

  1. Is it OK to run intellij-yarn-pnp-deps-tree-loader.js from IDE as node --require /path/to/.pnp.js intellij-yarn-pnp-deps-tree-loader.js? It works, but I'd like to ensure it's a legitimate way to run a script in Yarn PnP environment.
  2. Is there a way to install an arbitrary Yarn 2 release (e.g. 2.0.0-rc18) using command line?

@arcanis
Copy link
Member

arcanis commented Jan 15, 2020

Is it OK to run intellij-yarn-pnp-deps-tree-loader.js from IDE as node --require /path/to/.pnp.js intellij-yarn-pnp-deps-tree-loader.js? It works, but I'd like to ensure it's a legitimate way to run a script in Yarn PnP environment.

Yep, we'll keep supporting non-PnP accesses to the API. The .pnp.js will also be the correct way to load the runtime for at least the 2.x line, and probably more (the only thing I could see that would make us change that are the builtin Node loaders, but that's very much a wip and it'll take years before we seriously get there).

Is there a way to install an arbitrary Yarn 2 release (e.g. 2.0.0-rc18) using command line?

Never thought about it 🤔 I think you should be able to download any release via GitHub:

https://github.com/yarnpkg/berry/raw/%40yarnpkg/cli/<version>/packages/yarnpkg-cli/bin/yarn.js

I'll add support for this in yarn set version too 👍 I'll also start publishing the releases on npm soon (by the end of the week, I'd say).

@arcanis
Copy link
Member

arcanis commented Jan 15, 2020

#696 will make it possible for non-PnP modules to access the PnP API based on their path.

That being said, if intellij-yarn-pnp-deps-tree-loader is just doing basic resolution, maybe you don't need the full PnP API and can just use createRequire? More details here (particularly the last snippet of the section).

@arcanis
Copy link
Member

arcanis commented Jan 15, 2020

Should be fixed in the latest release (minus the set version thing), closing 👍

@arcanis arcanis closed this as completed Jan 15, 2020
@segrey
Copy link
Contributor Author

segrey commented Jan 16, 2020

@arcanis Great, thanks! 👍 I've replaced const pnp = require('pnpapi'); with const pnp = require('module').findPnpApi(process.cwd()); and it works (see the result intellij-yarn-pnp-deps-tree-loader.js).

if intellij-yarn-pnp-deps-tree-loader is just doing basic resolution, maybe you don't need the full PnP API and can just use createRequire?

Not sure createRequire can be a replacement, intellij-yarn-pnp-deps-tree-loader uses getDependencyTreeRoots, getPackageInformation and some other API.

Regarding createRequire, what's the difference from node's require.resolve(request[, options])?
Seems I can also access dependencies using it. For example:

// test.js
const jestResolvedPath = require.resolve('jest', {paths: ['/home/sergey/WebstormProjects/untitled23']});
console.log(jestResolvedPath);
console.log(require.resolve('jest-cli', { paths: [jestResolvedPath] }));

Output:

pwd
/home/sergey/WebstormProjects/untitled23

node --require ./.pnp.js ../test.js
/home/sergey/WebstormProjects/untitled23/.yarn/cache/jest-npm-24.9.0-8ddb425e99-1.zip/node_modules/jest/build/jest.js
/home/sergey/WebstormProjects/untitled23/.yarn/cache/jest-cli-npm-24.9.0-67cda48cb4-1.zip/node_modules/jest-cli/build/index.js

@arcanis
Copy link
Member

arcanis commented Jan 16, 2020

Regarding createRequire, what's the difference from node's require.resolve(request[, options])?

I'm not 100% sure myself to be honest 😄 I think there is a stronger semantic meaning with createRequire, which means that the runtime may have more informations that lead to the proper resolution. I also think Yarn might have a bug with paths, so I will need to dig into it at some point ... generally speaking, I'd recommend to use createRequire if possible.

Btw, off topic but is ESLint supported out of the box with Yarn 2 and IntelliJ? Some engineers here have reported me that they had to use our SDK + generate an extra shim for eslint/lib/options.js + configure WebStorm to explicitly set the ESLint path; do you know if that's expected (and similar question for TypeScript, I think)?

@segrey
Copy link
Contributor Author

segrey commented Jan 16, 2020

Thanks for the explanation.
Yes, I can reproduce the issue with not working ESLint too. The problem is that require fails on full path to module if it's executed from a file located outside of workspace. For example, the following code will fail if it's run outside of workspace:

require('/home/segrey/WebstormProjects/untitled17/.yarn/cache/eslint-npm-6.8.0-d27045f313-1.zip/node_modules/eslint/lib/options');

@arcanis Is it possible to support again requiring by full path like that?

@arcanis
Copy link
Member

arcanis commented Jan 16, 2020

tldr: You need to go through createRequire, otherwise the require call is ambiguous.

I've thought some more about your question on require.resolve, and I now have a better answer which will also answer this other question.

First, consider that Yarn supports a mode where all projects on the disk load packages from the same global cache. Because of this, a problem appears when doing the following:

require('/home/segrey/WebstormProjects/untitled17/.yarn/cache/eslint-npm-6.8.0-d27045f313-1.zip/node_modules/eslint/lib/options');

Which dependency tree is this ESLint file part of? Since the cache may be shared, multiple projects on the disk may depend on this file - but we don't know which one is the right one if we only have the path (and we need to know it in order to give ESLint access to its own dependencies). We could maybe default to the global PnP hook, but even then it might not be the right one ... and generally speaking, if something cannot be guaranteed to be true, we should assume it can't be relied.

So a followup question is: how is Yarn able to make such require calls work from scripts located within a dependency tree? The answer is that it keeps track of the PnP API currently in use in each module, so when you make a require it will not only use the path you pass as parameter, but also the dependency tree of the script that calls require (as a trivia, this information is available in module.pnpApiPath). With both of those informations, we can disambiguate ESLint and be sure that we load the right version.

So one last question remains: what's the difference between createRequire and require.resolve(..., {paths})? The answer is that you don't actually use require.resolve alone ... you typically use it followed by a require call, right? And as we've seen, the require calls use the context of the caller script in order to disambiguate the dependency, which means that you'll always require ESLint as if it was part of the dependency tree of the caller (so in your case, the classic node_modules resolution). Which isn't right.

By contrast, createRequire is different because it actually creates a new module with a new context. This new context will locate the right PnP API given an entry point, and because you'll use the result of createRequire for both resolution and instantiation, you'll load the following modules from the proper context.

To make things maybe clearer, consider what happens if you run this code from a PnP project when the global cache is enabled?:

const pathToMyESLint = require.resolve('eslint');
const pathToAnotherESLint = require.resolve('eslint', {
  paths: [pathToAnotherProject],
});

// Since we have a global cache, pathToMyESLint === pathToAnotherESLint. So
// what should happen when we do this?
const eslint = require(pathToAnotherESLint);

By contrast, if you use createRequire, then we keep all the informations we need to disambiguate the calls:

const requireForOtherProject = createRequire(pathToAnotherProject + '/package.json');

const pathToMyESLint = require.resolve('eslint');
const pathToAnotherESLint = requireForOtherProject.resolve('eslint');

// pathToMyESLint and pathToAnotherESLint are still equal, but this time we are
// able to load them through their own unique `require` contexts:
const myEslint = require(pathToMyESLint);
const anotherESlint = requireForOtherProject(pathToAnotherESLint);

I hope that makes sense - it's fairly complex, so please feel free to ask me any question. I will try not to make my answers as long as this post 😅

@segrey
Copy link
Contributor Author

segrey commented Jan 17, 2020

@arcanis Thank you very much for the great explanation! It's clear now.
Regarding TypeScript, it's not supported currently (https://youtrack.jetbrains.com/issue/WEB-42637). BTW, is there any ETA for Yarn 2 release?

@arcanis
Copy link
Member

arcanis commented Jan 17, 2020

Yep, I plan to tag stable and make a release post next Friday! :)

Regarding TypeScript, it's not supported currently (https://youtrack.jetbrains.com/issue/WEB-42637).

Note that I recently implemented transparent TS support (the difference with native being that Yarn is silently patching the TS package to merge the minimal change set required for TS to have PnP support). It seems to work with WebStorm just fine, the only thing is that it currently requires to manually set the TypeScript path:

image

@segrey
Copy link
Contributor Author

segrey commented Jan 17, 2020

Do you mean that WebStorm could detect .../pnpify/typescript path and show it in a drop-down list?

@arcanis
Copy link
Member

arcanis commented Jan 17, 2020

No, I meant that maybe you could load it the same way as you load ESLint without needing an explicit shim (by doing the -r .../.pnp.js setup)?

@segrey
Copy link
Contributor Author

segrey commented Jan 17, 2020

Ah, I see, we'll take a look, thanks!

SergeyZh pushed a commit to JetBrains/intellij-plugins that referenced this issue Jan 22, 2020
SergeyZh pushed a commit to JetBrains/intellij-plugins that referenced this issue Jan 22, 2020
…B-43396)

yarnpkg/berry#693 (comment)
(cherry picked from commit 0f16a5968cc4f631be104938f568f0ac1a2c969c)

GitOrigin-RevId: 86c01dc91d5c7064470ded20d32eaef85f4f2d09
@remorses
Copy link

I have just encountered this, with the same repro as in the original issue (or even using yarn node)

This is a big issue because i am unable to use a local package that tries to use require('pnpapi') for pnp support

@remorses
Copy link

remorses commented Nov 5, 2020

@arcanis i made a reproduction at https://github.com/remorses/yarn-reproduction-pnpapi-from-file-located-outside-workspace

Should i open another issue or can we reopen this one?

@merceyz
Copy link
Member

merceyz commented Nov 5, 2020

Your repro behaves as expected, the file isn't controlled by a pnpapi so there is no pnpapi to resolve back to it

@arcanis
Copy link
Member

arcanis commented Nov 5, 2020

Depending on the context, you may explicitly require the pnpapi from another file:

const {createRequire} = require(`module`);
const otherModuleRequire = createRequire(`/path/to/another/file`);
const pnpApi = otherModuleRequire(`pnpapi`);

@remorses
Copy link

remorses commented Nov 5, 2020

How can I run a local binary (outside the workspace) that makes use of pnpapi inside a yarn workspace? I already tried yarn link and link: protocol without success

@remorses
Copy link

remorses commented Nov 5, 2020

I managed to use my local package publishing the package to a local registry with verdaccio and installing with yarn publish --force --registry http://localhost:4873 --access restricted to overwrite version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants