`npm copy` copy module files and dependencies into a deployable folder #4082

everett1992 · 2021-11-22T18:19:53Z

This idea was discussed in npm/rfcs#493 and the 2021-11-17 RFC meeting. The gist is there's a requirement to create a production copy of a package or workspace for deployment (zip archives for lambda, COPY into docker image) and existing npm commands have shortcomings, specifically regarding workspaces.

This draft CR will give a foundation for discussions.

what scripts, if any should be run when a workspace package is copied?
should this command respect --omit and --production flags?
could pack, install or another command support this usecase instead of adding a new command?
what should this command or flag be called?

I'll work on tests and documentation after more of the command is finalized

Goals (still up for discussion):

support workspace and non workspace packages
in a workspace a subset of workspace packages can be copied
command should not install new packages from the registry, instead it should manipulate the existing node_modules directory
command should copy install script generated files (should not need to run install scripts for packages copied from node_modules)

wraithgar · 2021-11-29T19:21:41Z

What is fs-extra providing that https://github.com/npm/fs isn't?

everett1992 · 2021-11-29T19:55:32Z

What is fs-extra providing that https://github.com/npm/fs isn't?

I used fs-extra because I couldn't find whatever fs lib npm preferred I wasn't aware of @npmcli/fs.

Is this a polyfil / monkey patch? Should I require fs or @npmcli/fs
@npmcli/fs isn't a dependency of npm/cli should I add it to package.json?
@npmcli/fs doesn't include recursive copy, fs.cp is Stability: 1 in node 16 and 17

wraithgar · 2021-11-29T20:05:41Z

* Is this a polyfil / monkey patch? Should I require fs or @npmcli/fs

It's a polyfill. Good examples of how we are using it are in cacache

* @npmcli/fs isn't a dependency of npm/cli should I add it to package.json?

It's already a subdependency of cacache so adding it to the top shouldn't change much.

* @npmcli/fs doesn't include recursive copy, `fs.cp` is Stability: 1 in node 16 and 17

Ah ok this would be the gap we'd need to fill in @npmcli/fs then.

everett1992 · 2021-11-29T20:32:17Z

Ah ok this would be the gap we'd need to fill in @npmcli/fs then.

Is that something you'll work on or should I start a PR?

wraithgar · 2021-11-29T21:06:29Z

Is that something you'll work on or should I start a PR?

Please start a PR. It's not something we'd be able to get to internally for some time.

everett1992 · 2021-11-29T21:16:46Z

Will you be happy with the experimental fs.cp interface?
Is there a recursive copy in npm dependencies or that I could add instead of contributing to @npmcli/fs? I'm not sure how much effort it is to implement a robust library-level recursive cp that matches fs.cp.

wraithgar · 2021-11-29T21:37:46Z

Yes matching what is likely to land in node itself would be the best option if you went down that path.

There's no current implementation of this functionality in npm itself. Arborist effects this by duplicating the tree itself and then reifying it to another destination. This obviously won't work for module files.

I believe that if you paired the recursive mkdir in @npmcli/fs with path.basename you could then iterate through everything packlist sends you, make sure the directory exists with the same ownership assumptions that npm already has baked into the other things it does, and use fs.cp as-is. It would leave yourself with a pretty small code footprint. It may be easier than implementing a recursive cp option that maintains parity w/ node.

everett1992 · 2021-12-01T18:19:55Z

I want to wait for more feedback on general implementation before I start working on replacing fs-extra.

I think we need a recursive copy. packlist can be replaced with mkdir -p and copy, but copying node_modules dependencies requires recursive copy. Maybe we could use arborist to reify a modified tree, but I would need help. I couldn't tell how to change the root of a tree.

I need `fs.cp` in `npm copy` to copy node_modules files. I'm adapting node's [lib/internal/fs/cp/cp.js][0]. I'm checking in the original so I can record changes in git. ref npm/cli#4082 [0]: https://github.com/nodejs/node/blob/1fa507f098ca7a89012f76f0c849fa698e73a1a1/lib/internal/fs/cp/cp.js

nlf · 2022-01-12T17:54:04Z

fwiw, @npmcli/fs does provide a recursive copy. it polyfills the recursive flag for fs.cp provided in newer node versions so that it will be usable in any version.

nlf · 2022-01-12T17:54:57Z

fwiw, @npmcli/fs does provide a recursive copy. it polyfills the recursive flag for fs.cp provided in newer node versions so that it will be usable in any version.

which i'm just now remembering was you that implemented! 🤦

jeanbmar · 2022-02-08T15:22:07Z

lib/commands/copy.js

+}
+
+async function relativeSymlink (target, path) {
+  await fs.mkdir(dirname(path), { recursive: true })


mkdir fails on Windows when the directory already exists, even with recursive: true

i've spent more time on this EEXIST issue, and it seems that pushing fs.cp calls inside tasks isn't safe. mkdir operations happen concurrently under the hood multiple times for a same folder, which results in an error

We can drop parallel tasks and just run sequentially

pose · 2022-06-07T19:28:46Z

Hi,

I was wondering if there were any news on this issue. @jeanbmar is there anything @everett1992 or I could help with to expedite this review?

Thanks,

raphaelboukara · 2022-07-07T02:47:41Z

Hi 😀, what's the status of this issue? thanks

pose · 2022-07-25T14:05:29Z

Hi @jeanbmar,

Sorry for the direct ping, do you have any updates on how we can make sure this PR moves forward?

Thanks again,

jeanbmar · 2022-07-25T16:07:05Z

Hi @pose, I don’t work with the npm team :). I need this too!

pose · 2022-07-26T09:12:23Z

Hi @nlf,

Sorry for the direct ping, we are trying to find the right owner for this review. Do you think you could help us out?

Thanks,

ghost · 2022-07-27T15:48:59Z

test/lib/commands/copy.js

+    },
+  })
+  process.chdir(npm.prefix)
+  npm.config.set('omit', ['optional'])


Is the name of the test correct? We omit not bundled, but optional dependencies here.

ghost · 2022-07-27T15:52:47Z

test/lib/commands/copy.js

+    npm.config.set('workspace', ['a'])
+    await npm.exec('copy', ['build'])
+
+    assertExists(path.join('build', 'node_modules', 'a'))


Maybe we should test for package.json and README.md copied, not just for the folder?

ghost · 2022-07-27T15:56:11Z

I can see that https://github.com/everett1992/rfcs/blob/npm-copy/accepted/0000-npm-copy.md mentions --production flag, but it is not covered in the code change. Is this intended?

everett1992 · 2022-08-03T16:24:45Z

@amazlite I haven't updated the RFC as npm has evolved. This PR uses the --omit flag rather than --production to match npm 7+.

npm install --production
npm WARN config production Use `--omit=dev` instead.

jeanbmar · 2022-08-07T10:24:57Z

@everett1992 I've created a repo to show people reading this PR how to use your work on npm copy while we are waiting for npm to move forward: https://github.com/jeanbmar/npm-copy-test.

I've noticed a problem though.
The RFC says that the command "copies the current project's files and dependencies to" the destination.
If you test my repo, you will notice that project's files aren't copied to destination. The current project is copied instead as a dependency in destination's node_modules.

jeanbmar · 2022-09-28T07:24:04Z

@everett1992 I've noticed an issue with symlinks.
To reproduce :

Install @tensorflow/tfjs-node in /opt/tfjs
Run npm copy /var/task from /opt/tfjs
Run ls -la /var/task/node_modules/@tensorflow/tfjs-node/deps/lib

Symlinks point to /opt/tfjs binaries

Symlinks are fine when doing mv /opt/tfjs/* /var/task/ instead of npm copy

jeanbmar · 2022-10-14T14:05:26Z

@everett1992 Have you lost interest in this PR?
@wraithgar @darcyclarke Sorry for direct ping, what's the status of this PR and what can we do to push it further?

everett1992 mentioned this pull request Nov 22, 2021

docs: add npm copy rfc npm/rfcs#493

Open

darcyclarke added Release 8.x work is associated with a specific npm 8 release semver:minor new backwards-compatible feature Needs Review pr: needs tests requires tests before merging Needs Discussion is pending a discussion labels Nov 30, 2021

everett1992 mentioned this pull request Dec 7, 2021

polyfill node's fs.cp. npm/fs#6

Merged

everett1992 force-pushed the copy branch from 9869596 to 9d7cca6 Compare December 16, 2021 01:17

everett1992 force-pushed the copy branch 2 times, most recently from 928384a to 06e0234 Compare January 4, 2022 18:03

everett1992 force-pushed the copy branch 3 times, most recently from 651dbbd to 7fc9416 Compare January 11, 2022 00:21

fritzy force-pushed the latest branch from 06bdcc2 to d8aac84 Compare January 13, 2022 20:43

wraithgar changed the base branch from latest to release-next January 26, 2022 20:44

jeanbmar reviewed Feb 8, 2022

View reviewed changes

wraithgar changed the base branch from release-next to latest March 9, 2022 18:24

jeanbmar mentioned this pull request Mar 21, 2022

electron-forge does not work with npm workspaces (npm v7) electron/forge#2306

Closed

3 tasks

darcyclarke removed Needs Discussion is pending a discussion Needs Review labels Mar 31, 2022

everett1992 force-pushed the copy branch 2 times, most recently from 3fd4909 to 4933c68 Compare April 4, 2022 22:34

darcyclarke mentioned this pull request Apr 6, 2022

Open RFC Meeting - Wednesday, April 6, 2022, 2:00 PM EST npm/rfcs#565

Closed

everett1992 force-pushed the copy branch from 4933c68 to ddf68b9 Compare April 18, 2022 17:02

feat: new npm copy command

4da0872

everett1992 force-pushed the copy branch from ddf68b9 to 4da0872 Compare April 18, 2022 17:17

everett1992 marked this pull request as ready for review April 18, 2022 18:03

everett1992 requested a review from a team as a code owner April 18, 2022 18:03

jeanbmar mentioned this pull request Jul 12, 2022

Packaging multi-services in monorepositories (e.g. with Lerna) serverless/serverless#9095

Open

7 tasks

ghost reviewed Jul 27, 2022

View reviewed changes

jeanbmar mentioned this pull request Sep 6, 2022

Serverless 3 Umbrella FormidableLabs/serverless-jetpack#292

Open

8 tasks

fritzy force-pushed the latest branch 2 times, most recently from 3037d35 to f3b0c43 Compare September 14, 2022 23:09

jeanbmar mentioned this pull request Sep 27, 2022

Symlinks are invalid after moving node_modules tensorflow/tfjs#6865

Closed

lukekarrys force-pushed the latest branch from 591d1d1 to 9e74d3e Compare October 19, 2022 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`npm copy` copy module files and dependencies into a deployable folder #4082

`npm copy` copy module files and dependencies into a deployable folder #4082

everett1992 commented Nov 22, 2021 •

edited

wraithgar commented Nov 29, 2021

everett1992 commented Nov 29, 2021

wraithgar commented Nov 29, 2021

everett1992 commented Nov 29, 2021

wraithgar commented Nov 29, 2021

everett1992 commented Nov 29, 2021

wraithgar commented Nov 29, 2021 •

edited

everett1992 commented Dec 1, 2021

nlf commented Jan 12, 2022

nlf commented Jan 12, 2022

jeanbmar Feb 8, 2022 •

edited

jeanbmar Feb 8, 2022 •

edited

everett1992 Feb 10, 2022

pose commented Jun 7, 2022

raphaelboukara commented Jul 7, 2022

pose commented Jul 25, 2022

jeanbmar commented Jul 25, 2022

pose commented Jul 26, 2022

ghost Jul 27, 2022

ghost Jul 27, 2022

ghost commented Jul 27, 2022

everett1992 commented Aug 3, 2022

jeanbmar commented Aug 7, 2022 •

edited

jeanbmar commented Sep 28, 2022

jeanbmar commented Oct 14, 2022

npm copy copy module files and dependencies into a deployable folder #4082

Are you sure you want to change the base?

npm copy copy module files and dependencies into a deployable folder #4082

Conversation

everett1992 commented Nov 22, 2021 • edited

wraithgar commented Nov 29, 2021

everett1992 commented Nov 29, 2021

wraithgar commented Nov 29, 2021

everett1992 commented Nov 29, 2021

wraithgar commented Nov 29, 2021

everett1992 commented Nov 29, 2021

wraithgar commented Nov 29, 2021 • edited

everett1992 commented Dec 1, 2021

nlf commented Jan 12, 2022

nlf commented Jan 12, 2022

jeanbmar Feb 8, 2022 • edited

Choose a reason for hiding this comment

jeanbmar Feb 8, 2022 • edited

Choose a reason for hiding this comment

everett1992 Feb 10, 2022

Choose a reason for hiding this comment

pose commented Jun 7, 2022

raphaelboukara commented Jul 7, 2022

pose commented Jul 25, 2022

jeanbmar commented Jul 25, 2022

pose commented Jul 26, 2022

ghost Jul 27, 2022

Choose a reason for hiding this comment

ghost Jul 27, 2022

Choose a reason for hiding this comment

ghost commented Jul 27, 2022

everett1992 commented Aug 3, 2022

jeanbmar commented Aug 7, 2022 • edited

jeanbmar commented Sep 28, 2022

jeanbmar commented Oct 14, 2022

`npm copy` copy module files and dependencies into a deployable folder #4082

`npm copy` copy module files and dependencies into a deployable folder #4082

everett1992 commented Nov 22, 2021 •

edited

wraithgar commented Nov 29, 2021 •

edited

jeanbmar Feb 8, 2022 •

edited

jeanbmar Feb 8, 2022 •

edited

jeanbmar commented Aug 7, 2022 •

edited