[experimental] yarn pnp #7726

jeysal · 2019-01-27T11:08:38Z

Edit: Just to be clear, I did not intend for this to be merged in this or some similar form 😄 this PR is just to share what I've done and what the problems are so we can possibly get back to this when there are good solutions for them :)

Summary

Since it seems nobody has attempted this before, I tried for about an hour to make the Jest repo Yarn pnp-compatible. My impression is that it would be possible (with a significant amount of additional effort put into it), but might not be desirable until pnp has matured further.

What works:

yarn, including post install
~90% of unit/e2e tests (although I achieved this in a quick and dirty way)
yarn lint, mostly

What doesn't:

yarn flow, this fix did not help either
ESLint has 13 no-unresolved errors because of dependencies that are correctly declared in the corresponding package.json, but not the root. I consider this to be quite a deal breaker, because I really do not want a dependency on react in the root
examples/react-native because of node_modules/something all over the Jest preset file, although @SimenB may have fixed that in fix: use require.resolve in jest-preset facebook/react-native#22972

I did not check any of the other scripts.

cc @arcanis in case you're interested, although probably none of this is new to you 😅

Test plan

jeysal · 2019-01-27T11:14:46Z

CI looks a bit worse than on my machine, it shouldn't fail that early 😄

SimenB · 2019-01-27T11:21:58Z

package.json

@@ -26,6 +27,7 @@
    "eslint-plugin-prettier": "^3.0.1",
    "eslint-plugin-react": "^7.1.0",
    "eslint-plugin-relay": "~0.0.19",
+    "estraverse": "^4.2.0",


SimenB · 2019-01-27T11:22:04Z

package.json

@@ -3,6 +3,7 @@
  "devDependencies": {
    "@babel/core": "^7.1.0",
    "@babel/plugin-transform-strict-mode": "^7.0.0",
+    "@babel/plugin-transform-modules-commonjs": "^7.1.0",


this should go on master

SimenB · 2019-01-27T11:22:09Z

package.json

    "prettier": "^1.16.1",
    "prettylint": "^1.0.0",
    "progress": "^2.0.0",
    "promise": "^8.0.2",
    "readable-stream": "^3.0.3",
+    "realpath-native": "^1.0.0",
+    "request": "^2.88.0",


if it's for fetchSupporters, it should go in website/package.json

SimenB · 2019-01-27T11:23:14Z

package.json

    "prettier": "^1.16.1",
    "prettylint": "^1.0.0",
    "progress": "^2.0.0",
    "promise": "^8.0.2",
    "readable-stream": "^3.0.3",
+    "realpath-native": "^1.0.0",


for the integration test? if so, should land on master

SimenB · 2019-01-27T11:23:54Z

jest.config.js

  setupFilesAfterEnv: ['<rootDir>/testSetupFile.js'],
  snapshotSerializers: [
    '<rootDir>/packages/pretty-format/build/plugins/ConvertAnsi.js',
-    'jest-snapshot-serializer-raw',
+    require.resolve('jest-snapshot-serializer-raw'),


we can land this on master

SimenB · 2019-01-27T11:24:39Z

e2e/runJest.js

+  const result = spawnSync(
+    JEST_PATH,
+    // TODO this is ugly and a problem for e2e tests related to config/resolution
+    ['--resolver', require.resolve('jest-pnp-resolver'), ...(args || [])],


yeah... no. Not sure how to properly deal with it, though

Just to be clear, I did not intend for this to be merged in this or some similar form 😄 this PR is just to share what I've done and what the problems are so we can possibly get back to this when there are good solutions for them :)

jeysal · 2019-01-27T11:32:04Z

@SimenB I'll make a PR for the changes you suggested to integrate into master - I guess revealing possible improvements like these would be one of the main benefits of pnp if we could use it at some point

jeysal · 2019-01-27T11:37:04Z

Regarding the other changes such as request for fetchSupporters, the reason why pnp would force us to put so many dependencies into the root is essentially this yarnpkg/yarn#6514 (comment).
This is the primary reason why I do not consider using pnp viable at this point.

arcanis · 2019-01-27T11:50:24Z

Really nice! I think it could be simplified if there was a way to remove the need for require.resolve on the user side. I have to check, but I wanted to look at jest-config/normalize - maybe it would simply be a matter of using require.resolve(config.someSettings, {paths:[path.dirname(configPath)]}), what do you think?

arcanis · 2019-01-27T11:52:21Z

Regarding the packages required at the root, can you list why they are needed? It's usually only a way to workaround a legit issue in a package, and maybe the right fix would simply be to propagate the fixes upstream.

jeysal · 2019-01-27T12:04:50Z

require.resolve(config.someSettings, {paths:[path.dirname(configPath)]})

I believe that should work. preset is a weird case though:
"A preset that is used as a base for Jest's configuration. A preset should point to an npm module that has a jest-preset.json or jest-preset.js file at the root."
But that will probably get cleaned up with the configuration overhaul in Jest 25

Regarding the packages required at the root, can you list why they are needed

I'll compile a non-exhaustive list, there would probably be a lot more cases of this to show up if we want to make all scripts work :)

jeysal · 2019-01-27T12:25:34Z

Here's a few:

If request is put into website/package.json devDependencies instead of the root, yarn postinstall prints:

error /Users/timseckinger/proj/jest/.pnp/unplugged/npm-jest-website-0.0.0/node_modules/jest-website: Command failed.
Exit code: 1
Command: node fetchSupporters.js
Arguments:
Directory: /Users/timseckinger/proj/jest/.pnp/unplugged/npm-jest-website-0.0.0/node_modules/jest-website
Output:
/Users/timseckinger/proj/jest/.pnp.js:27718
    throw firstError;
    ^

Error: You cannot require a package ("request") that is not declared in your dependencies (via "/Users/timseckinger/proj/jest/website/fetchSupporters.js")

Not sure why this is happening.

If estraverse is omitted, yarn lint fails everything with:

  10:40  error  Parse errors in imported module 'types/Config':
Package "eslint@5.12.0" (via "/Users/timseckinger/Library/Caches/Yarn/v4/npm-eslint-5.12.0-fab3b908f60c52671fb14e996a450b96c743c859/node_modules/eslint/lib/api.js")
is trying to require the package "estraverse" (via "estraverse") without it being listed in its dependencies
(@babel/code-frame, ajv, chalk, cross-spawn, debug, doctrine, eslint-scope, eslint-utils, eslint-visitor-keys, espree, esquery, esutils, file-entry-cache, functional-red-black-tree, glob, globals, ignore, import-fresh, imurmurhash, inquirer, js-yaml, json-stable-stringify-without-jsonify, levn, lodash, minimatch, mkdirp, natural-compare, optionator, path-is-inside, pluralize, progress, regexpp, semver, strip-ansi, strip-json-comments, table, text-table, eslint)
(undefined:undefined)  import/namespace

This might be an error on ESLint side and thus actionable?

If realpath-native is in e2e/package.json dependencies instead of the root, yarn jest hasteMapSize fails with:

  ● Test suite failed to run

    Cannot find module 'realpath-native' from 'hasteMapSize.test.js'

      12 | import HasteMap from 'jest-haste-map';
      13 | import {cleanup, writeFiles} from '../Utils';
    > 14 | import {sync as realpath} from 'realpath-native';
         | ^
      15 | 
      16 | const DIR = path.resolve(realpath(os.tmpdir()), 'haste_map_size');
      17 | 

      at Resolver.resolveModule (packages/jest-resolve/build/index.js:202:17)
      at Object.<anonymous> (e2e/__tests__/hasteMapSize.test.js:14:1)

Not sure why the custom Jest pnp resolver doesn't manage to resolve this

SimenB · 2019-01-27T13:08:23Z

Not sure why this is happening.

Seems like an pnp bug, @arcanis?

This might be an error on ESLint side and thus actionable?

Yeah, send them a PR

Not sure why the custom Jest pnp resolver doesn't manage to resolve this

Yeah, weird. Seems like something it should handle

jeysal · 2019-01-27T13:49:59Z

This might be an error on ESLint side and thus actionable?

Actually, looks like this is babel/babel-eslint#680, which was fixed in babel-eslint@10

SimenB · 2019-01-27T14:51:25Z

Needs facebook/fbjs#330 to avoid the peer dep warning, then

jeysal · 2019-02-13T21:16:11Z

Closing this for now (was mostly meant to document the issues encountered while experimenting anyway), will attempt this again once pnp has matured enough to support complex setups / has become more of a standard way of doing dependencies.

arcanis · 2019-03-20T17:47:58Z

Current notes from today's session:

Workspaces are unplugged when they have a postinstall script (Yarn bug)
Unblock: remove the postinstall
TSC doesn't support PnP
Unblock: disable the buildTs step
Running yarn add in a subdirectory corrupts the PnP file (Yarn bug)
Unblock: run yarn install after the add
The Jest tests are using PnP when running within the PnP process
Unblock: should be reworked to disable the PnP native support from within the tests
The prettierPath doesn't go through require.resolve unless user-specified (not 100% sure)
Unblock: set "prettierPath: 'prettier'" in jest.config.js

jeysal · 2019-03-20T18:13:03Z

https://github.com/jeysal/jest/tree/yarn-pnp has the fixes and workarounds, I'll land some of the fixes in Jest master as well

SimenB · 2020-01-28T12:04:20Z

I opened #9476 using berry, if you wanna poke at it again 🙂

github-actions · 2021-05-11T11:06:44Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

WIP yarn pnp

7d73223

facebook-github-bot added the cla signed label Jan 27, 2019

SimenB reviewed Jan 27, 2019

View reviewed changes

jeysal mentioned this pull request Jan 27, 2019

dependency and resolution cleanup #7729

Merged

jeysal closed this Feb 13, 2019

jeysal mentioned this pull request Mar 9, 2019

Adds native support for PnP to jest #8094

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[experimental] yarn pnp #7726

[experimental] yarn pnp #7726

jeysal commented Jan 27, 2019 •

edited

jeysal commented Jan 27, 2019

SimenB Jan 27, 2019

SimenB Jan 27, 2019

SimenB Jan 27, 2019

SimenB Jan 27, 2019

SimenB Jan 27, 2019

SimenB Jan 27, 2019

SimenB Jan 27, 2019

jeysal Jan 27, 2019

jeysal commented Jan 27, 2019

jeysal commented Jan 27, 2019

arcanis commented Jan 27, 2019

arcanis commented Jan 27, 2019

jeysal commented Jan 27, 2019 •

edited

jeysal commented Jan 27, 2019

SimenB commented Jan 27, 2019

jeysal commented Jan 27, 2019

SimenB commented Jan 27, 2019

jeysal commented Feb 13, 2019

arcanis commented Mar 20, 2019 •

edited

jeysal commented Mar 20, 2019

SimenB commented Jan 28, 2020

github-actions bot commented May 11, 2021

[experimental] yarn pnp #7726

[experimental] yarn pnp #7726

Conversation

jeysal commented Jan 27, 2019 • edited

Summary

Test plan

jeysal commented Jan 27, 2019

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

Choose a reason for hiding this comment

jeysal commented Jan 27, 2019

jeysal commented Jan 27, 2019

arcanis commented Jan 27, 2019

arcanis commented Jan 27, 2019

jeysal commented Jan 27, 2019 • edited

jeysal commented Jan 27, 2019

SimenB commented Jan 27, 2019

jeysal commented Jan 27, 2019

SimenB commented Jan 27, 2019

jeysal commented Feb 13, 2019

arcanis commented Mar 20, 2019 • edited

jeysal commented Mar 20, 2019

SimenB commented Jan 28, 2020

github-actions bot commented May 11, 2021

jeysal commented Jan 27, 2019 •

edited

jeysal commented Jan 27, 2019 •

edited

arcanis commented Mar 20, 2019 •

edited