Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Implement Yarn 2 support #1630

Closed
wants to merge 15 commits into from
Closed

Implement Yarn 2 support #1630

wants to merge 15 commits into from

Conversation

constgen
Copy link
Contributor

@constgen constgen commented Oct 3, 2020

Resolves #1277

  • Bump minimal Jest version to support Plug and Play
  • Add PnP middleware
  • Make sure all resolves are PnP compatible
  • Use PnP middleware in all presets
  • Adopt all ESLint presets as there are known problems with ESLint sharable configs and PnP

@constgen
Copy link
Contributor Author

constgen commented Oct 3, 2020

I can see in the build logs that the job published packages to NPM. But why? It even didn't pass the code formatting check. And this is only Pull Request.

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for the PR.

I much prefer Yarn over NPM and have wanted PnP support for a while.

That said:

  • My understanding is that webpack 5 supports Yarn PnP out of the box, so much of this may be redundant in Neutrino 10+
  • Jest needs updating to a minor newer version (but bumping the peer dep is a breaking change), so we'd have to have a docs element to this anyway
  • This implementation adds the pnp-webpack-plugin dependency and config complexity to NPM users not just yarn

As such, I'm slightly reluctant to add a new middleware for Neutrino 9 only, which would be removed in Neutrino 10 - and think it might make sense for this to be a new section in the docs for Neutrino 9, that contains a snippet for people to add to their .neutrino.js along with some suggestions as to Jest upgrades etc.

Then in Neutrino 10 with webpack 5, this could just be supported natively with little implementation needed on Neutrino's side.

Thoughts?

packages/jest/package.json Outdated Show resolved Hide resolved
@edmorley
Copy link
Member

I can see in the build logs that the job published packages to NPM. But why?

It's not a real NPM server - see:
https://github.com/neutrinojs/neutrino/blob/master/scripts/test-create-project-ci.sh

@edmorley edmorley marked this pull request as draft October 12, 2020 09:10
@constgen
Copy link
Contributor Author

constgen commented Oct 12, 2020

Here are my thoughts.
I have my personal set of Neutrino middlewares and presets for React, ESLint and other. I reuse couple of Neutrino core middlewares, but most are custom. I faced really a lot of problems with Yarn 2. Especially with ESlint. I resolved them. Some solutions were really hacky. So I decided to integrate my experience to the Neutrino core.

Lets discuss your points

  • PnP will be supported in Webpack 5. This is true. Until this moment configuration need to use PnPPlugin. Some projects need this already today. Internally the plugin checks the PnP environment (Yarn documented how to do this) and enables necessary functionality on demand. Speaking simply it has no effect on NPM and PNPM projects. I also thought about conditionally enabling it but realized that it is easier to inspect Webpack Config when it is always included. This is how CRA and Storybook do - they always include PnPPlugin to configuration.
  • I will document the integration of Jest and PnP. But please recommend me where to write "PnP" section
  • I thought that Neutrino goal is to hide the config complexity from the consumer. That's why I decided that react and vue presets should support this out of the box without thinking how to enable this. And according to the first point it should not affect the build process on NPM. Again, CRA does this. Also it is little easier to upgrade to Webpack 5 later. You will not need to edit .neutrinorc.js

Comment on lines 25 to 31
if (pnpApi) {
resolvedPluginPath = pnpApi.resolveRequest(pluginName, relativeFilename);
} else {
resolvedPluginPath = require.resolve(pluginName, {
paths: [relativeFilename],
});
}
Copy link

Choose a reason for hiding this comment

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

You should just use require.resolve you almost never need to use the pnpapi directly, this is one of those cases

Copy link
Contributor Author

@constgen constgen Nov 3, 2020

Choose a reason for hiding this comment

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

I know. But unfortunately __filename gives a virtual File System path in Yarn 2 which can't work correctly in paths options of require.resolve. Module will be not found with an exception. Struggled with this 2 days.

Copy link

Choose a reason for hiding this comment

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

Do you have a reproduction for that? We have tests making sure it works. I could see that happening if you have enableGlobalCache: true but that will be fixed in yarnpkg/berry#2068

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any place of your code in Yarn 2 environment try this console.log(require.resolve('any-of-your-dependency', { paths: [__filename] })). It will not find your module even if it is relative from the current file. The same code works without any problems in regular NPM environment

Copy link

@merceyz merceyz Nov 3, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@constgen constgen Nov 6, 2020

Choose a reason for hiding this comment

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

I realized that the mentioned test case works in the Yarn 2 project itself. But in the linked module it fails with not found dependency. As you understand I use linked packages for local environments. Such modules are defined as

{
 "devDependencies": {
   "@neutrinojs/eslint": "portal:C:/Projects/Projects_GitHub/neutrino-dev-fork/packages/eslint",
  }
}

yarn install is called to setup all links in .yarn/cache and .pnp.js
And after that I can run local script "lint": "eslint ./src --ext .js,.jsx", to run the code and test the case

Copy link

Choose a reason for hiding this comment

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

Yeah, that makes more sense. I'm fairly certain the issue you're running into was fixed in yarnpkg/berry#2068, could you try with yarn set version from sources or by downloading the bundle here https://github.com/yarnpkg/berry/actions/runs/346381133?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before: Yarn version v2.2.2
After: Yarn version 2.3.3-git.20201112.f5ca1c57
Result is the same in the linked package: works when require.resolve('babel-eslint') and fails when require.resolve('babel-eslint', { paths: [__filename] })

Oops! Something went wrong! :(

ESLint: 7.9.0

Error: Cannot read config file: C:\Users\const\Desktop\yarn-test\.eslintrc.js
Error: Cannot find module 'babel-eslint'

Copy link

Choose a reason for hiding this comment

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

Could you try using __dirname instead?
Again, if you could provide a reproduction I'll be able to look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update:
After removing .yarn/cache and reinstalling dependencies require.resolve('babel-eslint', { paths: [__filename] }) started to work. It was the version 2.3.3-git.20201112.f5ca1c57.
Than I upgraded Yarn to the latest 2.4.0 and reinstalled dependencies again. And it still works. So I removed usage of PnP API as you asked

@constgen
Copy link
Contributor Author

constgen commented Nov 3, 2020

Please, look at the new section in the documentation https://github.com/constgen/neutrino-dev/tree/pnp/packages/eslint#utility-functions

@constgen
Copy link
Contributor Author

I think everything related to Yarn 2 is resolved.
The only left are tests. And I think there is a separate problem. I feel like tests can affect each other and they are not isolated. Seems like they may have a shared neutrino config and all Webpack plugins are applied across test. Is it possible? Because currently just commenting one test can make other tests red or green.

@constgen constgen marked this pull request as ready for review December 1, 2020 21:17
@constgen constgen changed the title [WIP] Implement Yarn 2 support Implement Yarn 2 support Dec 1, 2020
@constgen
Copy link
Contributor Author

constgen commented Dec 1, 2020

@edmorley in Webpack 5 only pnp-webpack-plugin will be not necessary. All other fixes to ESLint, Jest and etc. will be highly required. Their issues still remain. So with pnp-webpack-plugin or without it we need this PR any way. But without pnp-webpack-plugin it will be hard to automate testing in Yarn 2. In my opinion it affects only several presets and it will be really easy to remove it in Neutrino 10 without affecting anything.

@constgen
Copy link
Contributor Author

constgen commented Mar 8, 2021

@edmorley @eliperelman can you look at final version of this PR?

@edmorley
Copy link
Member

edmorley commented Mar 8, 2021

@constgen Thank you for working on this :-) Before it can be merged (and so before spending time on review), we need to have CI passing, which is blocked on Travis CI credits running out, due to Travis CI's new policy (that is, there are no more free credits). The best way to resolve this would be to migrate to another provider -- I would recommend Circle CI. I don't have time to work on such a migration, but would be happy to review a PR that did so.

@constgen
Copy link
Contributor Author

constgen commented Mar 9, 2021

Is GitHub Actions is an option? I tried it recently for one of my projects as a replacement for Travis. Have to say that it also has free plan limits but it worked well for me

@edmorley
Copy link
Member

edmorley commented Mar 9, 2021

Yeah that works too :-)

@edmorley edmorley removed their request for review April 26, 2022 08:36
@edmorley
Copy link
Member

edmorley commented Feb 3, 2024

Closing since unfortunately this project is no longer maintained. See:
#1707
neutrinojs/webpack-chain#358

@edmorley edmorley closed this Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

Add support for Yarn's Plug n Play mode
3 participants