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

[RRFC] Add flag for running NPM commands in transitive dependencies #548

Open
Tracked by #699
zackerydev opened this issue Mar 4, 2022 · 20 comments
Open
Tracked by #699

Comments

@zackerydev
Copy link

Motivation ("The Why")

There are many tools to manage multiple NPM packages in a concise and user friendly way. For a long time the front runner for those were Lerna and Yarn Workspaces.

NPM mono repos tools usually have two main features:
- Installing node_modules across an entire tree of dependencies and symlinking any dependencies in the tree that are found to be local to the mono repo
- Running commands across those packages

With the release of NPM 7 and NPM workspace npm itself came much closer to having the features needed out of the box to run a mono repo. This was great because Lerna has since been largely abandoned and the Yarn ecosystem was fragmented with the release of v2. However, NPM workspaces is lacking in the ability to run commands across transitive dependencies.

There are other mono repo tools gaining popularity like TurboRepo and NX but those have their own toolchains and features to consider, I think it would be preferred for NPM to support this out of the box.

It is important to note that this feature was expected to function this way by enough users that two issues npm/cli#3413 and npm/cli#4139 both considered this a bug and not a new feature.

Example

Having this feature would allow for mono repo tooling to run exclusively with npm something that comes out of the box with Node.js. Not requiring any other toolchains or special workflows.

How

Current Behaviour

Take this repo for example:
https://github.com/zgriesinger/monorepo

In it we have a npm workspace with frontends, apis, cli tools and shared packages.

If I pull the app down and:

npm i
npm run build -w @zgriesinger/service-a

I'll be met with errors since the @zgriesinger/logger package has not been built (but it has been symlinked)

If I know the transitive dependencies of service-a that are local to the repo I can do this to get the build working

npm run build -w @zgriesinger/logger
npm run build -w @zgriesinger/service-a

This is why most mono repos have to keep some other toolchain around, with lerna there is the --include-dependencies flag that will automatically run the build for @zgriesinger/logger

Desired Behaviour

The desired behavior of the npm cli would be to include a --include-dependencies flag similar to lerna.

The functionality/algorithm of this flag would be as follows:

  • Read dependencies of targeted workspace with -w flag
    • If dependencies includes any internal to the mono repo, read them and start from the beginning (Including matching versions)
    • If not, run the desired npm run script in that package, and return up to run the command in the parent package

The result should be in the example above:

npm run build -w @zgriesinger/service-a
  -> Runs build in @zgriesinger/logger
  -> On succes runs build in @zgriesinger/service-a

References

@bnb
Copy link

bnb commented Apr 6, 2022

+1 to this behind a flag.

@ljharb
Copy link
Contributor

ljharb commented Apr 6, 2022

Per discussion, this seems analogous to lerna's --include-dependencies flag.

A separate RFC would perhaps be needed (or, maybe it's just considered a bug) that if A depends on B, and i explicitly npm run whatever -w a -w b, that npm should just "figure out" that B's script needs to run before A's script does, instead of in parallel.

@zackerydev
Copy link
Author

Linking for Prior Art:
https://turborepo.org/blog/turbo-1-2-0#new-task-filtering-api

I like this api much more than Lerna's --include-dependencies flag but the include dependencies paradigm might be more familiar for existing lerna users.

@diegohaz
Copy link

A separate RFC would perhaps be needed (or, maybe it's just considered a bug) that if A depends on B, and i explicitly npm run whatever -w a -w b, that npm should just "figure out" that B's script needs to run before A's script does, instead of in parallel.

Yeah! I'm not sure this RFC covers npm/cli#4139. But #442 was closed in favor of this, so I guess that should be part of it.

@darcyclarke
Copy link
Contributor

Leaving this on the agenda again for today & also just wanted to reference wireit (ref. https://github.com/google/wireit)

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label May 18, 2022
@wraithgar
Copy link
Member

This is an important update from a previous request to run scripts in topological order npm/cli#3034 (comment)

@niemyjski
Copy link

We really need this. Without this I'm forced to use lage or some other package manager on top of workspaces just to build properly.

@marijnh
Copy link

marijnh commented Mar 1, 2023

Is there currently any way to influence the build order of workspace packages? The npm docs themselves don't seem to say anything about this. Some 3rd party online resources claim you have to specify the workspaces in the right order in your package.json, but that seems to have no effect. I tried to read the source but failed to track the data back far enough to see where the order comes from.

I have a workspace in which some packages need bin scripts from others to build, and npm install on a fresh checkout seems to reliably pick one of the packages that depends on others to build first (even though it appears after its dependencies in the workspaces array). Until this RFC is implemented, does anyone know a stop-gap trick to get a setup like this to work?

(Even some way to tell npm to not run the workspace's install scripts and just set up the node_modules tree without deleting it on failure would already be helpful, so I can use npm to link up the workspaces and install external dependencies, and then run the build steps with my own script.)

@cbhdalton
Copy link

@marijnh Defining the workspaces in the specific order they need to be built is how my team is working around this. After swapping from:

"workspaces": [
    "src/**/frontend"
]

to something like:

"workspaces": [
    "src/core/frontend",
    "src/products/frontend",
    "src/**/frontend"
]

The core and products workspaces started getting built before the rest.

I'm far from an expert on npm though, so I doubt I could help you troubleshoot why it's not working in your project other than maybe suggesting you check that your package-lock.json file isn't overriding your changes.

@marijnh
Copy link

marijnh commented Mar 1, 2023

Defining the workspaces in the specific order they need to be built is how my team is working around this.

Interesting. That's what I am doing, but the sixth package in the list is having its install script run before any of the others (using npm 9.5.0).

Is this ordering documented anywhere that you know of (so I can report this behavior as a bug)?

@cbhdalton
Copy link

Ahh, possible slight misunderstanding on what you were trying to do on my end. I can't find any documentation on how the behavior of npm install changes, if at all, with regards to what order workspaces are defined.

What originally lead me to this thread was that the package.json file in the root of our project has a build script that runs npm run build --workspaces, and each of our workspaces then have their own build scripts that do whatever they need to do. We were running into issues with the individual build scripts requiring other workspaces to have their build scripts ran first and npm wasn't able to determine the correct order that these should be executed in based on the dependencies in the workspaces. I found this section of the npm docs that talks about running commands in workspaces, and defining the workspaces more specifically like I mentioned before solved our problem with the script order.

Just as a side note, it does look like I'm running a bit of an older version of npm (8.5.5).

I'm not sure if it'd work, but maybe you could try defining your own install script in the workspaces that just run an npm install, then maybe npm run install --workspaces would install them in the right order?

@marijnh
Copy link

marijnh commented Mar 1, 2023

That, unfortunately, also didn't help.

darryltec pushed a commit to GetJobber/atlantis that referenced this issue Mar 20, 2023
Basically, npm would run `prepare` on each packages in alphabetical order but components need design to be built first.

This works around that limitation by firing the scripts manually through `npm run [script] -w` which respects the order under `workspaces`

npm/rfcs#548
darryltec pushed a commit to GetJobber/atlantis that referenced this issue Mar 22, 2023
Basically, npm would run `prepare` on each packages in alphabetical order but components need design to be built first.

This works around that limitation by firing the scripts run script manually through `npm run [script] -w` which respects the order under `workspaces`

npm/rfcs#548
darryltec pushed a commit to GetJobber/atlantis that referenced this issue Mar 24, 2023
* build: use NPM workspace instead of Lerna

* fix: workaround npm's lack of topological oder

Basically, npm would run `prepare` on each packages in alphabetical order but components need design to be built first.

This works around that limitation by firing the scripts manually through `npm run [script] -w` which respects the order under `workspaces`

npm/rfcs#548

* fix: reorder scripts for my own sanity

* build: update circle to not look for lock files

* build: use node 18 image on circle ci

* fix: js and css linters

* build: use -omit=optional

* fix: update time test work on node 18

* fix: don't update snaps

* fix: update some snaps

* build: make storybook run on node 18

Thanks @rodrigoeidelvein for the workaround!

* fix: reinstall ts-node

* chore: use lerna recommended setting for symlink

* fix: refix storybook by using js file

* fix: match * on preventManualRelease check
darryltec pushed a commit to GetJobber/atlantis that referenced this issue Mar 24, 2023
* build: update nvmrc to use 18

* build: repackage package locks

* build: temporarily set legacy-peer-deps to true

* build: Use NPM Workspace instead of Lerna to bootstrap (#1142)

* build: use NPM workspace instead of Lerna

* fix: workaround npm's lack of topological oder

Basically, npm would run `prepare` on each packages in alphabetical order but components need design to be built first.

This works around that limitation by firing the scripts manually through `npm run [script] -w` which respects the order under `workspaces`

npm/rfcs#548

* fix: reorder scripts for my own sanity

* build: update circle to not look for lock files

* build: use node 18 image on circle ci

* fix: js and css linters

* build: use -omit=optional

* fix: update time test work on node 18

* fix: don't update snaps

* fix: update some snaps

* build: make storybook run on node 18

Thanks @rodrigoeidelvein for the workaround!

* fix: reinstall ts-node

* chore: use lerna recommended setting for symlink

* fix: refix storybook by using js file

* fix: match * on preventManualRelease check

* docs: Node 18 update readme (#1149)

* Update versions in readme

* Update engines in package.json

* fix: reset package-lock

* chore: strict engine and package lock

* docs: Fix cloudflare Pages deploy (#1150)

* Trying build

* Correcting variables

* Removed resource class

Co-authored-by: Darryl Tec <[email protected]>

---------

Co-authored-by: Kingston Fung <[email protected]>
Co-authored-by: Darryl Tec <[email protected]>

* fix: update package-lock.json

---------

Co-authored-by: Michael Paradis <[email protected]>
Co-authored-by: Kingston Fung <[email protected]>
@boeckMt
Copy link

boeckMt commented Mar 29, 2023

Leaving this on the agenda again for today & also just wanted to reference wireit (ref. https://github.com/google/wireit)

@darcyclarke are there any updates on this agenda or do you suggest to use something like wireit, TurboRepo or NX?

@darcyclarke
Copy link
Contributor

@boeckMt I'm unfortunately no longer working at GitHub or managing the npm CLI team. That said, I do believe the team still runs weekly "office hours" calls which are a bit different then the old RFC meetings but you can ask questions & get insight/feedback there every Wednesday (ref. https://github.com/npm/rfcs#when)

@justinfagnani
Copy link

@marijnh it sounds like you may be be interested in Wireit which is attempting to be an incremental addition to npm scripts that at its most basic allows for dependencies between scripts. Wireit preserves the usage of the npm CLI, and can be added incrementally for just the scripts that need it.

@V-iktor
Copy link

V-iktor commented Apr 27, 2023

This sounds like something that would solve my Docker builds.

When I build a specific app, if a workspace depends on another workspace, the build cannot find the dependencies inside the app/workspace:*/workspace:* package.json

@koga73
Copy link

koga73 commented May 24, 2023

This is definitely something that is needed. Without it we are forced to use a separate library. Any update on this?

@hperrin
Copy link

hperrin commented Jun 12, 2023

I just ran into this issue too with my TypeScript builds failing in workspace packages. I'm migrating from Lerna, so I already had Lerna set up, and I solved this by just using lerna run to run scripts instead of npm run.

I moved all my build steps from "prepare" to "prepublish", so they don't run with "npm i". I'd say this is more important for install than run for me.

@mvhenten
Copy link

mvhenten commented Jul 5, 2023

For folks looking for workarounds - simple pre- and post- hooks won't work due to npm/cli#3598 unfortunately. My use-case is a group of packages depending on each other's typescript compilation - ideally executed in prepare and in deterministic order.

Because I control consumers, for the time being I've moved some of the compile steps to postinstall where they will be a no-op but in real life this is a significant adoption blocker for npm workspaces in typescript projects.

@mahnunchik
Copy link

Any news? It is really helpful feature.

foxriver76 added a commit to ioBroker/ioBroker.js-controller that referenced this issue Jan 10, 2024
…orrect build script when building dependency packages

- and using normal workspace flag works but is ineffective and can lead to errors due to still open npm/rfcs#548
foxriver76 added a commit to ioBroker/ioBroker.js-controller that referenced this issue Jan 28, 2024
)

* prevent issue when values with stateNs.null exist in the database

- closes #2579

* rm log

* rm anoither log

* allow path aliases in adapter package

* rm types from validator again

* move tsc-alias to postbuild

* use lerna to build project again as npm workspaces does not use the correct build script when building dependency packages

- and using normal workspace flag works but is ineffective and can lead to errors due to still open npm/rfcs#548

* update jsonl
foxriver76 added a commit to ioBroker/ioBroker.js-controller that referenced this issue Jan 28, 2024
)

* prevent issue when values with stateNs.null exist in the database

- closes #2579

* rm log

* rm anoither log

* allow path aliases in adapter package

* rm types from validator again

* move tsc-alias to postbuild

* use lerna to build project again as npm workspaces does not use the correct build script when building dependency packages

- and using normal workspace flag works but is ineffective and can lead to errors due to still open npm/rfcs#548

* update jsonl
foxriver76 added a commit to ioBroker/ioBroker.js-controller that referenced this issue Jan 29, 2024
* optimize the upload procedure (#2589)

- closes #2538

* fixed problem that cb is never called on mh enable/disable (#2557)

* fixed problem that cb is never called on mh enable/disable

* fix type

* added enhanced adapter license info to schema and types (#2591)

* added license info

* fix update license script

* format

* deprecate common.license and introduce common.licenseInformation

* rm local schema from controller iopack

* bring back license for adapter tests

* WIP placeholder added

* prevent issue when values with stateNs.null exist in the database (#2580)

* prevent issue when values with stateNs.null exist in the database

- closes #2579

* rm log

* rm anoither log

* allow path aliases in adapter package

* rm types from validator again

* move tsc-alias to postbuild

* use lerna to build project again as npm workspaces does not use the correct build script when building dependency packages

- and using normal workspace flag works but is ineffective and can lead to errors due to still open npm/rfcs#548

* update jsonl

* ts back to v4, accidentaly chosen by resolving merge conflict

* build local

* clean install to try to solve install module not found

* another pack-lock update

* fix linter
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

No branches or pull requests