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

project initialization has dependency on yarn "classic" #3478

Open
climbertjh opened this issue Mar 24, 2024 · 18 comments
Open

project initialization has dependency on yarn "classic" #3478

climbertjh opened this issue Mar 24, 2024 · 18 comments
Labels
bug Something isn't working

Comments

@climbertjh
Copy link

climbertjh commented Mar 24, 2024

When creating a new projen project, the default packageManager for the NodeProject (and sub-classes) is set to NodePackageManager.YARN_CLASSIC which represents yarn "classic".

This results in a run of yarn install --check-files during the initialization of the project. This, because of the command-line invocation parameters, implies a subtle dependency on yarn "classic" being the version of yarn which is in-effect when npx projen new is run.

To re-create the problem, install yarn "modern" (use corepack enable and then run yarn set version stable in a folder above where the new project will reside). Create the folder for the new project, change directory to that folder, and run npx projen new typescript. An initial npm install will be performed and then the .projenrc.ts file will be generated and synthesized. At this point, an install task is run for the new project. This is where a second install is performed and, because the default packageManager is set to use yarn "classic", the install command run is yarn install --check-files. When the version of yarn that is configured is not yarn "classic", this install fails and the npx projen new typescript initialization steps fails.

@climbertjh
Copy link
Author

A work-around is available:

  • use npx projen new typescript --no-post to skip the post-synthesize install step
  • edit the .projenrc.ts file and add in the packageManager parameter to the project configuration options
  • run an npm install to install the required packages that have already been defined in package.json
  • then run npx projen to pick up the changes to .projenrc.ts.

It is the initial bootstrapping of the new project which has this dependency on yarn "classic" being installed.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 24, 2024

You can run this:

projen new typescript --package-manager=yarn_berry

Or maybe all uppercase YARN_BERRY, you need to check.

@climbertjh
Copy link
Author

Thanks @mrgrain . I will try that.

I would have tried it last night if that option were listed in the npx projen new --help output. Having to read the projen source code to learn about command-line parameters is kinda frustrating.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 24, 2024

You get the full help per project type with this:

npx projen new typescript --help

But it's not very obvious right now that you can do that.

@climbertjh
Copy link
Author

Well ... that gets further ... but still no joy.

Here's what that attempt shows me:

❯ npx projen new typescript --package-manager YARN_BERRY
👾 Project definition file was created at /home/tjh/Projects/repos/ncfour/test-project/.projenrc.ts
👾 Installing dependencies...
👾 install | yarn install
➤ YN0000: · Yarn 4.0.1
➤ YN0000: ┌ Resolution step
➤ YN0085: │ + @types/jest@npm:29.5.12, @types/node@npm:18.19.26, and 681 more.
➤ YN0000: └ Completed in 3s 183ms
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 541ms
➤ YN0000: ┌ Link step
➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental
➤ YN0000: └ Completed in 0s 747ms
➤ YN0000: · Done with warnings in 4s 656ms
👾 unable to resolve version for @types/jest from installed modules
👾 unable to resolve version for eslint-import-resolver-typescript from installed modules
👾 unable to resolve version for eslint-plugin-import from installed modules
👾 unable to resolve version for jest from installed modules
👾 unable to resolve version for ts-jest from installed modules
👾 unable to resolve version for ts-node from installed modules
👾 unable to resolve version for typescript from installed modules
👾 Installing dependencies...
👾 install | yarn install
➤ YN0000: · Yarn 4.0.1
➤ YN0000: ┌ Resolution step
➤ YN0085: │ - @ampproject/remapping@npm:2.3.0, @babel/compat-data@npm:7.24.1, @babel/core@npm:7.24.3, and 350 more.
➤ YN0000: └ Completed in 0s 366ms
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 248ms
➤ YN0000: ┌ Link step
➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental
➤ YN0000: └ Completed
➤ YN0000: · Done with warnings in 0s 813ms

> test-project@0.0.0 eslint
> npx projen eslint

Error: Command failed: npm run eslint --if-present
👾 eslint | eslint --ext .ts,.tsx --fix --no-error-on-unmatched-pattern  src test build-tools projenrc .projenrc.ts
/bin/sh: 1: eslint: not found
👾 Task "eslint" failed when executing "eslint --ext .ts,.tsx --fix --no-error-on-unmatched-pattern  src test build-tools projenrc .projenrc.ts" (cwd: /home/tjh/Projects/repos/ncfour/test-project)

    at checkExecSyncError (node:child_process:890:11)
    at Object.execSync (node:child_process:962:15)
    at exec (/home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/lib/util.js:15:19)
    at initProject (/home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/lib/cli/cmds/new.js:349:25)
    at Object.handler (/home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/lib/cli/cmds/new.js:69:36)
    at /home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/node_modules/yargs/build/index.cjs:1:8993
    at j (/home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/node_modules/yargs/build/index.cjs:1:4956)
    at _.handleValidationAndGetResult (/home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/node_modules/yargs/build/index.cjs:1:8962)
    at _.applyMiddlewareAndGetResult (/home/tjh/.npm/_npx/80cfb0dc84733c29/node_modules/projen/node_modules/yargs/build/index.cjs:1:9604) {
  status: 1,
  signal: null,
  output: [
    null,
    null,
    <Buffer f0 9f 91 be 20 1b 5b 33 37 6d 1b 5b 34 6d 65 73 6c 69 6e 74 1b 5b 32 34 6d 20 7c 20 65 73 6c 69 6e 74 20 2d 2d 65 78 74 20 2e 74 73 2c 2e 74 73 78 20 ... 326 more bytes>
  ],
  pid: 82049,
  stdout: null,
  stderr: <Buffer f0 9f 91 be 20 1b 5b 33 37 6d 1b 5b 34 6d 65 73 6c 69 6e 74 1b 5b 32 34 6d 20 7c 20 65 73 6c 69 6e 74 20 2d 2d 65 78 74 20 2e 74 73 2c 2e 74 73 78 20 ... 326 more bytes>
}

Note that npx projen new typescript and npx projen new typescript --package-manager NPM DO WORK. And I was actually more interested in using npm than yarn "modern" - so I am good for now. However, I think there is still some bug(s) to be figured out here.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 25, 2024

Thanks for testing this out. This is indeed a new error now. Seems like the eslint binary cannot be found and/or for some reason the commands npx projen eslint and npm run eslint --if-present don't work.

@mrgrain mrgrain added the bug Something isn't working label Mar 25, 2024
@mrgrain
Copy link
Contributor

mrgrain commented Mar 25, 2024

@mrgrain
Copy link
Contributor

mrgrain commented Mar 25, 2024

Okay it seems like we never changed the default linker in yarnBerryOptions to be "node-modules"

@climbertjh
Copy link
Author

climbertjh commented Mar 26, 2024

So, a bit more testing. Good news - I was able to get it working with a carefully set environment variable. Bad news - it looks like the default path creates a .yarnrc.yml file that is NOT valid YAML syntax (!)

A path that works:

mkdir test-project
cd test-project
yarn set version stable
export YARN_NODE_LINKER=node-modules  # this is the "trick"
npx projen new typescript --package-manager YARN_BERRY

I tried initializing a .yarnrc.yml file rather than setting the environment variable. That doesn't work.

And the bad news - after finishing up the initialization, the resulting .yarnrc.yml file contains:

# ~~ Generated by projen. To modify, edit .projenrc.ts and run "npx projen".

{}

which I didn't think was valid YAML syntax.

That said - yarn commands continue to work.

Adding the following to my .projenrc.ts, I was able to re-write the .yarnrc.yml file to set up the nodeLinker option:

project.tryRemoveFile(".yarnrc.yml");
new javascript.Yarnrc(project, "4.1.1", {
  nodeLinker: javascript.YarnNodeLinker.NODE_MODULES,
});

I hope this helps identify what fixes to make.

@climbertjh
Copy link
Author

One more note - npx projen new using a custom project type in a locally stored package also works.

For example:

npx projen new --from @ncfour/projen-utils@file:../projen-utils/dist/js/projen-utils@0.0.0.jsii.tgz typescript-esm --package-manager YARN_BERRY

also works (with YARN_NODE_LINKER environment variable set to node-modules).

@mrgrain
Copy link
Contributor

mrgrain commented Mar 26, 2024

FWIW, this is valid YAML because any JSON is valid YAML.

# ~~ Generated by projen. To modify, edit .projenrc.ts and run "npx projen".

{}

@climbertjh
Copy link
Author

Thanks @mrgrain - until today I didn't know that.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 26, 2024

Moving forward, I think we need to change the default setting for the linker. @blimmer what do you think ?

@mrgrain
Copy link
Contributor

mrgrain commented Mar 26, 2024

Fundamentally I think projen's concept of providing an env for task to run in, is reaching its limits now. With Yarn PNP, the only way to run scripts properly seems to be via yarn exec. It's a bit similar to poetry run, although for that a workaround using PATH exists.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 26, 2024

Or I suppose we could state that projen the task runner is not compatible with Yarn PnP and one must use Yarn as task runner in this case.

@climbertjh
Copy link
Author

How about just using yarn exec in the task definitions when packageManager is NodePackageManager.YARN_BERRY?

This would also serve to self-document working in the same "style" as the package manager/tool-chain choice either made or presumed by the user.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 27, 2024

How about just using yarn exec in the task definitions when packageManager is NodePackageManager.YARN_BERRY?

This would also serve to self-document working in the same "style" as the package manager/tool-chain choice either made or presumed by the user.

The problem with this approach is that every Component now has to be aware of the package manager used and has to actively support it. For example the Esbuild Component, currently it will look like this:

project.addTask('esbuild', 'esbuild --fix');

That's nice and simple and portable.

If the component needs to wrap the command with yarn exec it turns into this monster:

assert(project instanceof NodeProject); // a bit constructed, but now this component cannot work with other projects anymore

switch(project.packageManager) {
  case NodePackageManager.YARN_BERRY:
  case NodePackageManager.YARN_CLASSIC:
    project.addTask('esbuild', 'yarn exec esbuild --fix');
    break;
  case NodePackageManager.PNPM:
    project.addTask('esbuild', 'pnpm exec esbuild --fix');
    break;
  case NodePackageManager.NPM:
  default:
    project.addTask('esbuild', 'npx esbuild --fix');
    break;
}

and every component would have to do this!


My proposal is essentially that the "wrapping" is done once by the task runtime.

@blimmer
Copy link
Contributor

blimmer commented Apr 12, 2024

I'm going to speak from the perspective of what we'd do in a perfect world, noting that this might be difficult or a large-ish overhaul of the code.

The current behavior of always using npx is not correct and assumes that npm is present and a compatible version. projen should always exec binaries provided by package.json (dev)Dependencies with the package manager being used. As mentioned in this thread, this will allow using different package management implementations that don't place binaries at node_modules/.bin. There are greater discussions ongoing about whether a package manager should be provided by default in the future.

Instead of offloading the addTask behavior to plugin authors, could we provide a wrapper that somehow handles that for them? Maybe we introduce a new method called .addTaskWithExec or .addTaskWithNodePath (bad names 😓) that handles this for you? Then all internal methods could use it and authors could migrate to the newer API?

We could even eventually deprecate .addTask and force authors to choose if their task is a "shell task" or a " exec task".

In the meantime, though, I don't see a huge issue with forcing the node_modules linker (or at least adding a warning that if you choose another linker you might have a bad time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants