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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds native support for PnP to jest #8094

Merged
merged 10 commits into from Mar 9, 2019
Merged

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Mar 9, 2019

Summary

PnP currently needs an extra plugin to work properly. Adding native would remove friction and strictly improve users' life (since PnP users can't really use Jest without the plugin anyway).

Test plan

I went to implement a e2e test but it proved challenging given the current test infra. The problem is that the tests run on the local Jest build (which makes sense), which doesn't use PnP. It makes it difficult to spawn a e2e test that would use it 馃槙

So I guess for now the main test is that it doesn't break non-PnP users.

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

Note that I've directly used the jest-pnp-resolver to make the diff simpler, but I'd be open to transfer the repo to the Jest org in a follow-up 馃檪

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Exciting!

Note that I've directly used the jest-pnp-resolver to make the diff simpler, but I'd be open to transfer the repo to the Jest org in a follow-up 馃檪

Yeah, that makes sense to me as a follow-up. I guess it's stable enough to not be hindered by Jest's slower release process?

I went to implement a e2e test but it proved challenging given the current test infra. The problem is that the tests run on the local Jest build (which makes sense), which doesn't use PnP. It makes it difficult to spawn a e2e test that would use it 馃槙

Is it possible to package up a tarball for PnP? We have bunch of tests that sets up temp directories in /tmp, runs yarn etc.

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

From CI:

error TS7016: Could not find a declaration file for module 'jest-pnp-resolver'. '/home/circleci/jest/node_modules/jest-pnp-resolver/index.js' implicitly has an 'any' type.
  Try `npm install @types/jest-pnp-resolver` if it exists or add a new declaration (.d.ts) file containing `declare module 'jest-pnp-resolver';

Maybe add a typings file to the project? If not, you can add a .d.ts file (similar to how e.g. node-notifier is currently)

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Another cool integration test would be that the project is usable with PnP in general, not just the resolver. I have nefarious plans to split up jest-config, and it would be nice to have a test yelling at me if I mess up the dependency tree. A test using jest-pnp-resolver shows that jest can be used to test PnP projects, but a separate one showing that a version of Jest installed in a PnP project can start at all is something else (I think that makes sense! 馃榾)

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

Found a way to make a test! I was worried that mixing PnP (the test env) and non-PnP (the Jest repo) wouldn't work, but PnP is smart enough to fallback to the regular Node resolution for the relevant parts of the tree when it detects this situation 馃檪

Yeah, that makes sense to me as a follow-up. I guess it's stable enough to not be hindered by Jest's slower release process?

I think so, yep. I haven't had to change the API in a while and I don't expect this to be very frequent. Most changes are in the PnP implementation, not its public interface.

Another cool integration test would be that the project is usable with PnP

You mean the whole Jest repo? If you're interested, we could look into making you early adopters of Yarn v2, that would be win-win! There are still probably some rough edges, but it would be instructive and I'd be happy to fix them.

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

You mean the whole Jest repo? If you're interested, we could look into making you early adopters of Yarn v2, that would be win-win!

I meant installing jest in some location and running it to ensure dependencies are correct.
But migrating to yarn v2 would be even cooler and assert on the same thing. I'm definitely down!

@jeysal
Copy link
Contributor

jeysal commented Mar 9, 2019

If you're interested, we could look into making you early adopters of Yarn v2

Hell yeah 馃敟
I made a quick attempt at using PnP in #7726 but there were issues in many different places so I decided to not invest further time in it.
I guess if PnP works, that should already be half the battle to make v2 work and uncover quite a few issues a large infrastructure project like Jest might have with it?
@arcanis will you be at FB London when the Jest summit takes place March 18-20? Even if Yarn v2 is still too alpha by then, we could probably get PnP working a lot quicker than I could on my own 馃槄

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

@arcanis will you be at FB London when the Jest summit takes place March 18-20? Even if Yarn v2 is still too alpha by then, we could probably get PnP working a lot quicker than I could on my own 馃槄

That's the week I'm in Menlo Park 馃槶馃槶

But I'd definitely be up to schedule a few hours for a peer coding session to get a better sense of the blockers together!

@jeysal
Copy link
Contributor

jeysal commented Mar 9, 2019

That's the week I'm in Menlo Park

Damn, unlucky ^^

But I'd definitely be up to schedule a few hours for a peer coding session to get a better sense of the blockers together!

Sounds great! I'll drop you a DM / mail next week or so :)

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI passes - Node 6 is probably just a trailing comma thing as usual? 馃槄

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

Indeed - should be fixed once #8095 is merged 馃憤

@jeysal
Copy link
Contributor

jeysal commented Mar 9, 2019

BTW, mind explaining to me why #8095 will fix this? Was there a bug in Yarn 1.13 when using Node 6?

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

Not exactly a bug, but PnP wasn't originally tested on Node 6 so the generated .pnp.js file used trailing commas. It got fixed in yarnpkg/yarn#6871.

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

Which reminds me btw that Yarn 2 will drop compatibility with Node 6, since its official EOL is in April. Will that be a problem?

https://github.com/nodejs/Release/blob/master/schedule.svg

@jeysal
Copy link
Contributor

jeysal commented Mar 9, 2019

Not exactly a bug, but PnP wasn't originally tested on Node 6 so the generated .pnp.js file used trailing commas. It got fixed in yarnpkg/yarn#6871.

Ah okay, thanks! I couldn't really interpret the CI error 馃槄

Which reminds me btw that Yarn 2 will drop compatibility with Node 6, since its official EOL is in April. Will that be a problem?

I think we intend to drop it in the next major after the EOL as well, so that should be fine. cc @SimenB

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Timing wise, maaaaaybe. But I wanna press pretty aggressively for Node 8 (so we can stop transpiling async, upgrade jsdom and use ink), so there shouldn't be a huge issue. But I'm very much a fan of moving to node 8 quickly

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Windows is failing

Error: Cannot find module 'D:\a\1\jest\e2e\pnp/.pnp.js'

@codecov-io
Copy link

codecov-io commented Mar 9, 2019

Codecov Report

Merging #8094 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8094      +/-   ##
==========================================
- Coverage   62.46%   62.45%   -0.01%     
==========================================
  Files         263      264       +1     
  Lines       10371    10374       +3     
  Branches     2514     2514              
==========================================
+ Hits         6478     6479       +1     
- Misses       3317     3318       +1     
- Partials      576      577       +1
Impacted Files Coverage 螖
packages/jest-resolve/src/defaultResolver.ts 64.06% <0%> (-2.07%) 猬囷笍
e2e/pnp/lib/index.js 100% <100%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 562ac18...80b65fb. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Still same error on azure :/

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

@SimenB I'll disable the test on Windows. It's hard to tell without starting a bunch of other runs just to get extra logs, but I think it's simply that on this specific Azure setup the Yarn cache (probably on the user folder in C:) is on a different drive than the git clone (which seems to be in D:). Since Yarn cannot generate relative paths between the two, the PnP codepath is disabled. It shouldn't affect end-users.

(For the record a more long-term fix is to configure the Yarn cache to be stored on the same disk, for example in the git repository itself, but I think that's a bit heavy for this PR)

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Sounds good

@arcanis
Copy link
Contributor Author

arcanis commented Mar 9, 2019

All green 馃憤


it('sucessfully runs the tests inside `pnp/`', () => {
// https://github.com/facebook/jest/pull/8094#issuecomment-471220694
if (process.platform !== 'win32') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a skipOnWindows helper

import {skipSuiteOnWindows} from '@jest/test-utils';

skipSuiteOnWindows();

Copy link
Member

@SimenB SimenB Mar 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to add it via GH

@SimenB SimenB merged commit c929ee3 into jestjs:master Mar 9, 2019
@billyvg
Copy link

billyvg commented Mar 13, 2019

I'm having issues with pnp + a setupFile that has imports that depend on a modulePath -- anyone else have a similar issue? You can repo here: https://github.com/getsentry/sentry/tree/build/travis/yarn-pnp

I can submit as a new issue if that's better

@SimenB
Copy link
Member

SimenB commented Mar 14, 2019

A new issue, yes 馃檪

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants