Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

feat(rebuild): add lifecycle scripts dependencies #303

Conversation

ruyadorno
Copy link
Contributor

Adds the ability for lifecycle scripts of Link nodes to depend
on each other, such that a prepare script of linked module A
can make use of files that are a result of a prepare script of
a linked package B.

This is important in order to unlock workflows in which a workspace
needs to make use of another workspace but they all have transpilation
steps (very common in Typescript projects) so tracking the order
(and completion) in which the dependencies lifecycle scripts runs
becomes necessary.

References

Fixes: npm/cli#3034

@ruyadorno ruyadorno requested a review from isaacs July 14, 2021 21:00
@ruyadorno ruyadorno force-pushed the ruyadorno/add-ability-for-link-scripts-to-depend-on-each-other branch from 094466d to 0e71ba8 Compare July 14, 2021 21:02
@ruyadorno ruyadorno changed the title feat(links): add lifecycle scripts interdependency feat(rebuild): add lifecycle scripts dependencies Jul 14, 2021
Adds the ability for lifecycle scripts of Link nodes to depend
on each other, such that a `prepare` script of linked module A
can make use of files that are a result of a `prepare` script of
a linked package B.

This is important in order to unlock workflows in which a workspace
needs to make use of another workspace but they all have transpilation
steps (very common in Typescript projects) so tracking the order
(and completion) in which the dependencies lifecycle scripts runs
becomes necessary.

Fixes: npm/cli#3034
@ruyadorno ruyadorno force-pushed the ruyadorno/add-ability-for-link-scripts-to-depend-on-each-other branch from 0e71ba8 to b4689eb Compare July 14, 2021 21:37
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

This encounters a deadlock when there is a cycle between link dependencies. Example to reproduce it:

diff --git a/test/arborist/rebuild.js b/test/arborist/rebuild.js
index 77c38623..53993201 100644
--- a/test/arborist/rebuild.js
+++ b/test/arborist/rebuild.js
@@ -790,6 +790,7 @@ t.test('scripts dependencies between Link nodes', async t => {
             // at least 20ms prior to writing that file
             prepare: "node -e \"setTimeout(() => { require('fs').writeFileSync(require('path').resolve('index.js'), 'module.exports = function HELLO() {}.name;') }, 20)\"",
           },
+          dependencies: { b: '' },
         }),
       },
       d: {

The need is clearly valid, but we have to be able to determine the case where a dependency is waiting on something that exists in a cycle. Also, I think we can probably address it without needing a class to manage references and polling? At the very least, the Script class here needs to be a standalone thing with unit tests.

@felschr
Copy link

felschr commented Jul 29, 2021

We hit this issue afer updating to npm@7.20 and npm install suddenly failing because it tries to run prepare scripts out of order. I'm happy to see there's already a PR in the pipeline.
With all the other recent additions npm workspaces are starting to become really useful, great work!

@alvis
Copy link

alvis commented Aug 12, 2021

Maybe what we need is a mechanism to detect a cyclic dependency and raise a warning?

Since 7.20, we can't use workspace with lerna anymore since npm would try to run prepare before leana bootstrap.
Besides the out-of-order prepare sequence, what make it worser is that npm i --ignore-scripts doesn't even stop npm from running the prepare scripts.

@wooorm
Copy link

wooorm commented Aug 26, 2021

Workaround for those who might run into it: explicitly list the order of items in:

  "workspaces": [
     "packages/used-by-others",
     "packages/others",
 ...

@fritzy
Copy link
Contributor

fritzy commented Jan 18, 2022

@ruyadorno Closing this in order to archive the repo.

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.

[BUG] Prepare scripts in workspaces should wait for dependencies
6 participants