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

feat(core): upgrade yarn v1 => v3 [berry] && replace npx with yarn dlx #7918

Closed
wants to merge 7 commits into from
Closed

feat(core): upgrade yarn v1 => v3 [berry] && replace npx with yarn dlx #7918

wants to merge 7 commits into from

Conversation

benpsnyder
Copy link
Contributor

@benpsnyder benpsnyder commented Nov 27, 2021

Current Behavior

Today you support Yarn v1 and there are numerous issues where people try to use Yarn v2 (berry) and sometimes Yarn v3, but most are unresolved. Then there are challenges with getting e2e to work (example: #7504 and srleecode/domain#30)

Expected Behavior

With this PR, you should expect Nx core to support Yarn v3 and generally replace npx with yarn dlx.

Regarding modifications to @nrwl/tao ... someone will have to touch up (pun intended) packages/tao/src/shared/package-manager.ts so it is cross-platform as the touch command will not execute on Windows for everyone. (EDIT: this has been done!).

Regarding modifications to @nrwl/nx-plugin ... I had to add --packageManager=yarn to the runNxNewCommand() function in packages/nx-plugin/src/utils/testing-utils/nx-project.ts for e2e to work. This should be modified with conditional logic, of course, since not everyone will be using Yarn. (EDIT: good progress has been made on this and is detailed below in the comments)

Related Issue(s)

Fixes #6509 and #7504 and #5672

@vercel
Copy link

vercel bot commented Nov 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/G8qJ7f8z4BuFKbtUFBGZNsfqXM5N
✅ Preview: Canceled

[Deployment for 0c4839f failed]

@benpsnyder
Copy link
Contributor Author

Also - I can't remember at the moment but we're using yarn set version 3.x because there is a bug in yarn set version berry

package.json Outdated
"check-versions": "ts-node -P ./scripts/tsconfig.scripts.json ./scripts/check-versions.ts",
"check-documentation-map": "ts-node -P ./scripts/tsconfig.scripts.json ./scripts/documentation/map-link-checker.ts",
"update-nx-dev-documentation": "ts-node -P ./scripts/tsconfig.scripts.json ./scripts/documentation/nx-dev-release-content.ts",
"check-internal-links": "npx ts-node -P ./scripts/tsconfig.scripts.json ./scripts/documentation/internal-link-checker.ts",
Copy link

Choose a reason for hiding this comment

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

You shouldn't mix Yarn and npm, using npx here is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merceyz I agree. How about I include yarn dlx as a replacement for npx? Let me know if you think this is fine.

Copy link

Choose a reason for hiding this comment

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

Just remove npx and call ts-node directly as it was the case in the removed lines. When run through yarn, it will automatically check the locally installed packages for a corresponding binary.

@@ -41,7 +41,7 @@ export function getPackageManagerCommand(
): PackageManagerCommands {
const commands: { [pm in PackageManager]: () => PackageManagerCommands } = {
yarn: () => ({
install: 'yarn',
install: 'yarn dlx touch yarn.lock && yarn install',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has now been fixed. It will work cross-platform for Yarn v3

@@ -15,7 +15,7 @@ function runNxNewCommand(args?: string, silent?: boolean) {
return execSync(
`node ${require.resolve(
'@nrwl/tao'
)} new proj --nx-workspace-root=${localTmpDir} --no-interactive --skip-install --collection=@nrwl/workspace --npmScope=proj --preset=empty ${
)} new proj --nx-workspace-root=${localTmpDir} --no-interactive --skip-install --collection=@nrwl/workspace --npmScope=proj --packageManager=yarn --preset=empty ${
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a hack and we need conditional logic ... I need some help with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Phillip9587 any chance you could give this a review, and lend some help? thx

Copy link

Choose a reason for hiding this comment

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

If you need to detect which package manager invoked the process you should use process.env.npm_config_user_agent.

yarn/3.1.1 npm/? node/16.13.0 linux x64
npm/7.20.1 node/v16.13.0 linux x64 workspaces/false
pnpm/6.11.0 npm/? node/v16.13.0 linux x64

Copy link
Contributor Author

@benpsnyder benpsnyder Nov 28, 2021

Choose a reason for hiding this comment

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

Excellent! I could implement that.

I'm also trying to get it to work using devkit functions .. here is what I've got so far but I am reading documentation as I am unfamiliar with the APIs.

// adding host:tree is a breaking change?
function runNxNewCommand(host: Tree, args?: string, silent?: boolean) {
  const localTmpDir = dirname(tmpProjPath());
  const packageManager = detectPackageManager(getWorkspacePath(host));
  
  // getWorkspacePath(host)) 
  console.log('Your package manager is:', detectPackageManager());

    return execSync(
      `node ${require.resolve(
        '@nrwl/tao'
      )} new proj --nx-workspace-root=${localTmpDir} --no-interactive --skip-install --collection=@nrwl/workspace --npmScope=proj --packageManager=${packageManager} --preset=empty ${
        args || ''
      }`,
      {
        cwd: localTmpDir,
        ...(silent && false ? { stdio: ['ignore', 'ignore', 'ignore'] } : {}),
      }
    );
  
}

Copy link
Contributor Author

@benpsnyder benpsnyder Nov 28, 2021

Choose a reason for hiding this comment

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

For anyone following along here... as of Nx 13.3.x ... without any changes to @nrwl/nx-plugin I get both a package-lock.json and yarn.lock in the e2e folder
image

We're trying to modify this runNxNewCommand() function to prevent that.

When adding --packageManager=yarn to runNxNewCommand() ... we do not get a package-lock.json file
image

And the added benefit of faster execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given
image

Fail when executing nx run e2e-domain-cypress-domain-test:e2e --verbose as process.env.npm_config_user_agent doesn't pick up on npm when nx is invoked directly
image

Success when executing: yarn nx run e2e-domain-cypress-domain-test:e2e --verbose
image

Success when executing: yarn dlx nx run e2e-domain-cypress-domain-test:e2e --verbose
image

Surprisingly... SUCCESS when executing: nx run e2e-domain-cypress-domain-test:e2e --verbose and npx nx run e2e-domain-cypress-domain-test:e2e --verbose
image

But then I figured out that it was just a false-positive because of Nx Cache. This fails: nx run e2e-domain-cypress-domain-test:e2e --verbose --skip-nx-cache
image

However, note that this passed npx nx run e2e-domain-cypress-domain-test:e2e --verbose --skip-nx-cache
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshots showing the appearance of package-lock.json and 'yarn.lock` given different commands

image

image

And note that pnpx fails.
image

I am going to push my code as-is so others can jump in on this.

Copy link
Contributor Author

@benpsnyder benpsnyder Nov 28, 2021

Choose a reason for hiding this comment

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

I think this is a better approach for packages/nx-plugin/src/utils/testing-utils/nx-project.ts ... I copied code from packages/create-nx-plugin

import { appRootPath } from '@nrwl/tao/src/utils/app-root';
import {
  getPackageManagerCommand,
  PackageManager,
  readJsonFile,
  writeJsonFile,
} from '@nrwl/devkit';
import { execSync } from 'child_process';
import { dirname } from 'path';
import { ensureDirSync } from 'fs-extra';
import { tmpProjPath } from './paths';
import { cleanup } from './utils';
import * as yargsParser from 'yargs-parser';

 // START copy functionality from: '../../../../create-nx-plugin/bin/detect-invoked-package-manager'
 // would like to import, but it needs to be refactored to @nrwl/devkit

 /**
  * Detects which package manager was used to invoke create-nx-{plugin|workspace} command
  * based on the main Module process that invokes the command
  * - npx returns 'npm'
  * - pnpx returns 'pnpm'
  * - yarn create returns 'yarn'
  *
  * Default to 'npm'
  */
 export function detectInvokedPackageManager(): PackageManager {
   let detectedPackageManager: PackageManager = 'yarn'; // changed from 'npm'
   // mainModule is deprecated since Node 14, fallback for older versions
   const invoker = require.main || process['mainModule'];
 
   // default to `yarn`
   if (!invoker) {
     return detectedPackageManager;
   }
 
   for (const pkgManager of ['pnpm', 'yarn', 'npm'] as const) {
     if (invoker.path.includes(pkgManager)) {
       detectedPackageManager = pkgManager;
       break;
     }
   }
 
   return detectedPackageManager;
 }

 // END copy
 //

function runNxNewCommand(args?: string, silent?: boolean) {
  const localTmpDir = dirname(tmpProjPath());

  // copied from: packages/create-nx-plugin/bin/create-nx-plugin.ts
  const parsedArgs: any = yargsParser(process.argv.slice(2), {
    string: [
      'name',
      'cli',
      'preset',
      'appName',
      'style',
      'defaultBase',
      'packageManager',
    ],
    alias: {
      packageManager: 'pm',
    },
    boolean: ['help', 'interactive', 'nxCloud'],
    default: {
      interactive: false,
    },
    configuration: {
      'strip-dashed': true,
      'strip-aliased': true,
    },
  }) as any;

  const packageManager: PackageManager =
  parsedArgs.packageManager || detectInvokedPackageManager();

  // amends the value to an string, even if its undefined or null etc
  console.log(packageManager);

  return execSync(
    `node ${require.resolve(
      '@nrwl/tao'
    )} new proj --nx-workspace-root=${localTmpDir} --packageManager=${packageManager} --no-interactive --skip-install --collection=@nrwl/workspace --npmScope=proj --preset=empty ${
      args || ''
    }`,
    {
      cwd: localTmpDir,
      ...(silent && false ? { stdio: ['ignore', 'ignore', 'ignore'] } : {}),
    }
  );
}

export function patchPackageJsonForPlugin(
  npmPackageName: string,
  distPath: string
) {
  const path = tmpProjPath('package.json');
  const json = readJsonFile(path);
  json.devDependencies[npmPackageName] = `file:${appRootPath}/${distPath}`;
  writeJsonFile(path, json);
}

/**
 * Generate a unique name for running CLI commands
 * @param prefix
 *
 * @returns `'<prefix><random number>'`
 */
export function uniq(prefix: string) {
  return `${prefix}${Math.floor(Math.random() * 10000000)}`;
}

/**
 * Run the appropriate package manager install command in the e2e directory
 * @param silent silent output from the install
 */
export function runPackageManagerInstall(silent: boolean = true) {
  const pmc = getPackageManagerCommand();
  const install = execSync(pmc.install, {
    cwd: tmpProjPath(),
    ...(silent ? { stdio: ['ignore', 'ignore', 'ignore'] } : {}),
  });
  return install ? install.toString() : '';
}

/**
 * Creates a new nx project in the e2e directory
 *
 * @param npmPackageName package name to test
 * @param pluginDistPath dist path where the plugin was outputted to
 */
export function newNxProject(
  npmPackageName: string,
  pluginDistPath: string
): void {
  cleanup();
  runNxNewCommand('', true);
  patchPackageJsonForPlugin(npmPackageName, pluginDistPath);
  runPackageManagerInstall();
}

/**
 * Ensures that a project has been setup in the e2e directory
 * It will also copy `@nrwl` packages to the e2e directory
 */
export function ensureNxProject(
  npmPackageName?: string,
  pluginDistPath?: string
): void {
  ensureDirSync(tmpProjPath());
  newNxProject(npmPackageName, pluginDistPath);
}

I don't have it working yet, but I think its the right path forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

@benpsnyder Sorry, I don't have any time at the moment...

@benpsnyder benpsnyder changed the title feat(core): implement Yarn v3 feat(core): implement Yarn v3 and replace npx with yarn dlx Nov 28, 2021
@benpsnyder benpsnyder changed the title feat(core): implement Yarn v3 and replace npx with yarn dlx feat(core): upgrade yarn v1 => v3 [berry] && replace npx with yarn dlx Nov 28, 2021
CONTRIBUTING.md Outdated
@@ -148,7 +148,7 @@ Note that adjusting the `schema.json` files will also affect the CLI manuals and
To run `nx-dev` locally, run the command:

```bash
npx nx serve nx-dev
yarn dlx nx serve nx-dev
Copy link

Choose a reason for hiding this comment

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

dlx will download-and-execute nx from the registry, that's probably not what you want here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what npx did as well. I personally don't see a problem with it, but understand your point.

Copy link

Choose a reason for hiding this comment

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

From npx's documentation:

Executes <command> either from a local node_modules/.bin, or from a central cache, installing any packages needed in order for <command> to run.

So actually it looks for a local version first.

Copy link
Contributor Author

@benpsnyder benpsnyder left a comment

Choose a reason for hiding this comment

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

@FrozenPandaz this is ready for review

@vsavkin vsavkin self-requested a review January 20, 2022 16:44
@vsavkin vsavkin self-assigned this Jan 20, 2022
@rhys-e
Copy link

rhys-e commented Feb 21, 2022

Hi, any update on this one? Would be great to use nx with yarn (PnP in particular solves the main issue of huge node_modules)

@ryanyue123
Copy link

Hi - would also love to know if there are any updates / expected timeline for this. Or if there is any way that we can help get this shipped faster!

@benpsnyder
Copy link
Contributor Author

I connected on Slack with Nrwlians and discussed this PR. I think it is wise to get node-linker w/ Yarn v3 in as Step 1, then go for PNP.

@Cammisuli would you be able to update the community here on the plans for this functionality? Thx

@@ -41,7 +41,7 @@ export function getPackageManagerCommand(
): PackageManagerCommands {
const commands: { [pm in PackageManager]: () => PackageManagerCommands } = {
yarn: () => ({
install: 'yarn',
install: 'yarn dlx touch yarn.lock && yarn install',
add: 'yarn add -W',
Copy link

@boeto boeto Mar 22, 2022

Choose a reason for hiding this comment

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

-W no longer used in the v3 version, so it should be : add: 'yarn add',

https://yarnpkg.com/cli/add#options

@@ -41,7 +41,7 @@ export function getPackageManagerCommand(
): PackageManagerCommands {
const commands: { [pm in PackageManager]: () => PackageManagerCommands } = {
yarn: () => ({
install: 'yarn',
install: 'yarn dlx touch yarn.lock && yarn install',
add: 'yarn add -W',
addDev: 'yarn add -D -W',
Copy link

@boeto boeto Mar 22, 2022

Choose a reason for hiding this comment

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

-W no longer used in the v3 version, so it should be : addDev: 'yarn add -D',

https://yarnpkg.com/cli/add#options

Copy link

@boeto boeto Mar 22, 2022

Choose a reason for hiding this comment

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

at the >13.9.0 version,the packages/tao/src/shared/package-manager.ts change to packages/nx/src/shared/package-manager.ts.

and a new parameter:addGlobal: 'yarn global add' was added to the >13.9.0 version.

yarn globalno longer used in the v3 version:https://yarnpkg.com/getting-started/migration#use-yarn-dlx-instead-of-yarn-global

@Cammisuli
Copy link
Member

There's a lot of stuff happening in the core of Nx - particularly with refactoring some packages and consolidating them into one. While this is going on, this PR will be woefully out of date, and will need to be restarted.

We would like to give yarn 2+ proper support, and this PR only introduces fixes for certain packages. We would eventually want to at least support using the nodeLinker: "node_modules plugin, and it should work with core nx.

I'm going to close this for now, and we will revisit it in the future.

@Cammisuli Cammisuli closed this Mar 25, 2022
@Cammisuli Cammisuli assigned vsavkin and unassigned Cammisuli Mar 25, 2022
@nemonemi
Copy link

nemonemi commented Mar 28, 2022

@Cammisuli,
I believe that simply saying "we would like to do it" and closing the issue is not really the answer.

This topic is more than a year and a half old and the PnP one is more than 2 years old.

Is there any plan on when yarn 2+ (and PnP) will be supported? Is there a roadmap of any sort?

@Cammisuli
Copy link
Member

Cammisuli commented Mar 28, 2022

I understand it's not the answer that everyone is hoping for, but it's a lot more work than expected. We shouldn't just patch one thing at a time to support it, it needs to be done properly with the whole team and community working together.

As for a timeline, I'm not too sure. Maybe @FrozenPandaz or @vsavkin can provide more insights on that.

@benpsnyder
Copy link
Contributor Author

Respectfully, I believe adopting Yarn v3 with node-linker is very possible as a short-term goal. It is as close are you get to Yarn v1, and it would move the ball forward. I can resubmit a clear PR for this if you like.

@pongells
Copy link
Contributor

@benpsnyder -- I guess you never submitted a clear PR for yarn3 with node linker? is it working out of the box now?

i.e. would the following be enough to have nx play nice with yarn3+node_modules?

  • add "packageManager": "yarn" in nx.json
  • update the yarn version to latest
  • update the .yarnrc.yml with nodeLinker: node-modules

@benpsnyder
Copy link
Contributor Author

@pongells that sounds about right and you need to update .gitignore (follow the docs on yarn's site).

Our organization moved to pnpm from Yarn v3 -- I recommend it

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

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

Successfully merging this pull request may close these issues.

Unable to successfully execute: nx migrate