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(npm): add flag for creating and resolving npm packages to a local node_modules folder #15971

Merged
merged 20 commits into from
Sep 22, 2022

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Sep 20, 2022

This flag is for greater compatibility with packages that depend on a local node_modules folder existing.

  1. Creates a local node_modules folder pnpm style -- Note: Missing peer dependency support for the first pass.
  2. The node_modules folder is created in the same folder as the resolved deno.json or the cwd if it doesn't exist.

Note: We cannot symlink to the global cache because that will cause resolutions to resolve to the global cache. For now, it copies, but we could hardlink if on the same file system in the future.

  • Bikeshed on --local-npm name. Also, Bartek recommended this maybe be an env var. Edit: It's now --node-modules-dir

Closes #15822

@dsherret dsherret mentioned this pull request Sep 20, 2022
25 tasks
ext/node/path.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

Bikeshed on --local-npm name. Also, Bartek recommended this maybe be an env var.

That's right, I think we should consider adding DENO_LOCAL_NODE_MODULES env variable instead. I'm not sure if one is better than the other, but I feel like env variable could be specified in the config file's "tasks" section so it doesn't have to be exposed to users as a flag. That said, David pointed out that the same is true with the flag.

@dsherret dsherret marked this pull request as ready for review September 21, 2022 15:48
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

A few comments. I think --node-modules-dir will be more intelligible flag name than --local-npm.

cli/fs_util.rs Show resolved Hide resolved
@@ -294,6 +324,60 @@ pub async fn remove_dir_all_if_exists(path: &Path) -> std::io::Result<()> {
}
}

/// Copies a directory to another directory.
///
/// Note: Does not handle symlinks.
Copy link
Member

Choose a reason for hiding this comment

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

Should it handle symlinks at some point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. For now it doesn't because it's not necessary for npm packages from my understanding.

cli/npm/resolvers/common.rs Show resolved Hide resolved
cli/npm/resolvers/local.rs Show resolved Hide resolved
cli/npm/resolvers/local.rs Show resolved Hide resolved
Comment on lines +1878 to +1882
let testdata_dir = testdata_path();
let args = args
.iter()
.map(|arg| arg.replace("$TESTDATA", &testdata_dir.to_string_lossy()))
.collect::<Vec<_>>();
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 21, 2022

While an env var might be more appropriate in the sense that typically node_modules isn't committed and is more of a local dev consideration, if the whole purpose is for when projects depend on it existing then a flag+config makes far more sense IMO.

@dsherret dsherret changed the title feat(npm): add --local-npm flag for creating a local node_modules folder feat(npm): add flag for creating a local node_modules folder Sep 21, 2022
@dsherret dsherret changed the title feat(npm): add flag for creating a local node_modules folder feat(npm): add flag for creating and resolving npm packages to a local node_modules folder Sep 21, 2022
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@dsherret dsherret merged commit 716005a into denoland:main Sep 22, 2022
@dsherret dsherret deleted the local_node_modules_folder branch September 22, 2022 15:17
dsherret added a commit that referenced this pull request Sep 22, 2022
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

Successfully merging this pull request may close these issues.

Add a flag to symlink "node_modules/" directory to CWD
3 participants