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

yarn 2 compatibility #933

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

yarn 2 compatibility #933

wants to merge 5 commits into from

Conversation

cspotcode
Copy link

@cspotcode cspotcode commented Nov 10, 2020

I'm starting to work on yarn 2 compatibility. I'll see how far I can get.

Try it out today

Yarn 2 users, you can try this branch today and let me know what does / does not work.

# Install this version of tsdx
yarn add tsdx@github:cspotcode/tsdx.git#yarn2
# Now you can use tsdx
yarn tsdx test

I am also working at this other PR so that I can push frequently for CI to run without spamming this thread: cspotcode#4

Changes I've had to make:

  • yarn2 is built with this bugfix: Fix "typesVersions" support in plugin-compat typescript patch yarnpkg/berry#2215
  • Dependencies
    • Upgraded jest dependencies to 25.4, because jest-resolve 25.3 does not declare one of its own dependencies; causes an error
    • Added type-fest because it is referenced by src/templates/react.ts
    • Added @jest/types because it is referenced by src/createJestConfig.ts
    • Upgraded ts-jest to avoid error: Could not find source file:

Related issues that may / may not need fixes

Simplifications since the last review

This is a list of undesirable changes from my initial submission that I have reverted

  • no more environment variable; we simply run yarn and use process.versions.pnp to detect yarn2
  • jest runs under pnp, no more spawning external yarn processes
  • no more need to add deps to test package.json's; we copy all deps from the root package.json automatically
  • no more hacky testing builds in an external process; we are back to simply require()ing them in-process
  • fixed a bug in yarn2's typescript patch, meaning less changes to package.json dependencies
  • stage-* directories are no longer being moved elsewhere

Related links: (note to self explain why they're relevant)
jestjs/jest#10847
facebook/create-react-app#5270

@vercel

This comment has been minimized.

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I only took a quick glance through here, but there already seem to be several issues. CI is also failing as bahmutov/npm-install does not support Yarn 2: bahmutov/npm-install#22

The Jest issue is not the only issue with Yarn 2 support, there should be several others. But the Jest issue will likely be solved in a better fashion with presets #634 (though that still needs testing). This approach seems different from Jest's defaults, which could cause inconsistencies and easy API breakage (and may itself be breaking).

package.json Outdated
@@ -96,9 +94,10 @@
"tiny-glob": "^0.2.6",
"ts-jest": "^25.3.1",
"tslib": "^1.9.3",
"typescript": "^3.7.3"
"typescript": "^3.8.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change and a TS upgrade to 4.0+ is scheduled for v0.16.0

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/createJestConfig.ts Outdated Show resolved Hide resolved
@@ -1,15 +1,30 @@
import { Config } from '@jest/types';
import { createRequire } from 'module';
import * as Path from 'path';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import * as Path from 'path';
import path from 'path';

That is how it is imported in several files, so please be consistent.

@cspotcode
Copy link
Author

This PR is marked draft because it is still a work in progress, so it's expected to be broken. I created it so I could ping @merceyz who wanted to follow along.

@cspotcode
Copy link
Author

I got all the tests passing in yarn2. PR is still marked "draft" because the yarn folks have sent a bugfix to jest that may allow me to simplify and speed up the changes I had to make here.

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Reply

This PR is marked draft because it is still a work in progress, so it's expected to be broken. I created it so I could ping @merceyz who wanted to follow along.

So a "Draft PR" indicates that it is "ready for feedback" but not necessarily ready for merging. If you did not intend to receive feedback, then it sounds like just having a branch fits the stage better than a Draft PR.

I was getting notifications of every commit here, so I had to unsubscribe to this thread entirely. Please note that you'll need to ping me directly for me to be notified as a result of that.

New Review

It looks like this is still in draft and there's still a number of remaining items left to-do, but I took a look at some of the code (not all of it; I haven't really read through some of the functions, shell scripts, or CI caching/steps etc) and left some comments. Some of the previous issues I mentioned in previous comments have not been resolved and there are some additional issues.

The scope of this PR, in particular, changing test infra and some test structure in non-trivial ways may have made sense for a pre-start discussion to reduce future iterations.

This will still need at least one more review and I'll have to thoroughly manually test it as well if all the issues are resolved.

Testing

It might be necessary to add some new tests or change something here, but I'm not sure. In particular,

This seems to only directly resolve the test portion with the createRequire code but not the others, so maybe there's something missing here, unless Berry has had some changes that make the other changes no longer required.

test/utils/fixture.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.github/workflows/nodejs.yml Outdated Show resolved Hide resolved
.gitignore Outdated
Comment on lines 10 to 15
/.yarn
/.yarnrc.yml
/.pnp.js
/test/yarn2/.pnp.js
/test/yarn2/yarn.lock
/test/yarn2/.yarn
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments as to why these are required would be very good to have. I actually have a branch adding comments to all the gitignores per #925 (comment)

Comment on lines 37 to 49
if (getPackageManager() === 'yarn2') {
// Setup stage directory as a project root for yarn2
fs.writeFileSync(
'.yarnrc.yml',
`yarnPath: ${path.join(
rootDir,
'test/yarn2/.yarn/releases/yarn-sources.cjs'
)}\ncacheFolder: ${rootDir}/test/yarn2/.yarn/cache`
);
// Copy seed lockfile with most resolutions already performed. Makes tests faster.
shell.exec(`cp ${rootDir}/test/yarn2/yarn.lock ${stagePath}/`);
}
packageManagerInstall();
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned in my above comment, the tests are not meant to install any new dependencies, that would slow tests down considerably, especially local testing. So any install code should not be done during the test stage, it should have already been installed by that stage.

Copy link
Author

Choose a reason for hiding this comment

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

The tests are running tsdx within yarn pnp, which means each stage directory needs to have its dependencies resolved. With running the stages in parallel, they each need to have their own PnP resolutions instead of sharing with the project root. Another option could be creating all stage-* directories at once, declaring them as workspaces of the root, and running yarn once to resolve them all.

This approach, running yarn in each stage directory, is still pretty fast thanks to the seed yarn.lock and the pre-populated cache. yarn essentially checks the yarn.lock, sees that it already knows the full dependency tree, sees that it already has all modules in cache, and can stop there.

const libTesterPath = require.resolve('./lib-tester.js');
const args: libTester.JsonArgs = { libPath: absLibPath, expression };
const encodedValue = shell.exec(
`${isYarn2 ? 'yarn ' : ''}node ${libTesterPath} ${Buffer.from(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't totally understand why the yarn node piece is necessary since the TSDX this runs, node ../dist/index.js build, is already built. The tests are also run with yarn test, so I would imagine PnP is already being used?

This is also fairly problematic with other work I have done and have planned to convert the E2E tests to unit tests -- see #407 (and related issues) which would allow, e.g. build as a function to be imported and used instead of node ../dist/index.js build.
Perhaps it would make sense to hold off on this until that is complete?

Related to that is in-memory filesystems; once that's done I can use memoryfs or something similar to run the tests significantly faster (fs I/O is one of the bottlenecks). I'm not sure if Berry will affect that too.

Copy link
Author

Choose a reason for hiding this comment

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

IIRC I set it up so that jest is still running out of yarn 1 node_modules (less changes to the tests) and only tsdx and the build output of tsdx are executed with PnP. yarn node is responsible for installing PnP into the node process by setting the right NODE_OPTIONS env var.

To support unit testing, we can switch to running everything under yarn 2. I tried that earlier on in my experimenting, so I can switch back to it.

test/utils/fixture.ts Outdated Show resolved Hide resolved
test/utils/fixture.ts Outdated Show resolved Hide resolved
const config: JestConfigOptions = {
transform: {
'.(ts|tsx)$': require.resolve('ts-jest/dist'),
'.(js|jsx)$': require.resolve('babel-jest'), // jest's default
'.(js|jsx)$': resolveBabelJest(), // jest's default
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per my first top-level comment, this no longer seems to be using "jest's default":

But the Jest issue will likely be solved in a better fashion with presets #634 (though that still needs testing). This approach seems different from Jest's defaults, which could cause inconsistencies and easy API breakage (and may itself be breaking).

@merceyz
Copy link

merceyz commented Nov 24, 2020

rollup-plugin-pnp-resolve wasn't added but is mentioned in #688 (comment) and #458 (comment)

resolve, which is used by @rollup/plugin-node-resolve, has PnP support so that rollup plugin isn't needed anymore.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@cspotcode
Copy link
Author

@agilgur5 thanks for taking a look, I'm working through the review feedback. I've left comments on a few things, am pushing fixes for some, and for other things like jest presets, I'll need to do some research.

It has been helpful to let Github Actions run the tests while I work, which is why I've been pushing code here as I go. Understanding that you've muted this thread, is it ok if I keep pushing code as I go? I can ping you only once I've addressed all review feedback.

@cspotcode
Copy link
Author

I got rid of the npm misnomer and trimmed down the test matrix.

I've reverted the package.json changes. I did some testing and they were only necessary when running jest under PnP. In another branch I'm going to re-implement the changes to run jest under PnP, which should simplify the tests, eliminate some of the messier changes to test infra, and enable unit-testing. I'll see if/how many dependency changes are necessary.

@timini
Copy link

timini commented Nov 26, 2020

Great to see so much progress on this, if there is a list of follow up tasks that would be great to get some others to input on those.

I mentioned before I would like to sponsor such efforts and I'd like to honour that, please sign up for issuehunt!

@cspotcode
Copy link
Author

cspotcode commented Dec 7, 2020

@timini how does issuehunt work? Do I need to sign up, or does the tsdx team need to sign up?

As far as getting other people involved with the remaining work, I added a checklist of tasks to the issue description, but at the moment it's a bit messy. I am also iterating in another branch and merging to this PR when major improvements are ready.
Trying to cut down on spam over here. cspotcode#4

Perhaps more useful than the checklist, I added instructions for installing and testing this version of tsdx today. Yarn 2 users, please feel free to give it a shot on your projects and let me know what's still broken.

@timini
Copy link

timini commented May 12, 2021

Tested creating a yarn 2 monorepo using workspaces. So far i have no issues, i had to setup a tsconfig.json at the workspace root which is extended by the workspace packages

https://github.com/timini/tsdx-yarn-2-monorepo

@timini
Copy link

timini commented May 12, 2021

I have done very minimal project setup so far, but it looked promising, its based off https://github.com/cspotcode/tsdx#yarn2

I haven't tried with main line tsdx.

Are there any tests i should make? Is it possible to add tests for this kind of monorepo setup?

@timini
Copy link

timini commented May 13, 2021

OK i tested the same setup using main branch tsdx and encountered issues with yarn install. It seems those issues are resolved by https://github.com/cspotcode/tsdx#yarn2

@cspotcode
Copy link
Author

@timini the "changed I've had to make" section of the OP lists fixes I made for compatibility. The yarn2 bugfix has since been merged and released, so shouldn't be necessary any more. Are you using the latest stable yarn 2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants