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(install): support npm specifiers #16634

Merged

Conversation

dsherret
Copy link
Member

Supports npm specifiers for deno install. This will by default always use a lockfile (which is generated on first run) unless --no-lock is specified.

worker.preload_main_module().await?;
let ps = ProcState::build(flags.clone()).await?;
// ensure the module is cached
load_and_type_check(&ps, &[install_flags.module_url.clone()]).await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

Seemed like a lot of ceremony. Can we just use this instead?

Copy link
Member

Choose a reason for hiding this comment

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

If the tests pass I think it's fine

@@ -189,7 +205,7 @@ pub fn uninstall(name: String, root: Option<PathBuf>) -> Result<(), AnyError> {

// There might be some extra files to delete
for ext in ["tsconfig.json", "lock.json"] {
file_path = file_path.with_extension(ext);
let file_path = file_path.with_extension(ext);
Copy link
Member Author

Choose a reason for hiding this comment

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

Bug here. It would do file.tsconfig.json then file.tsconfig.lock.json. Now that's fixed.

@@ -995,10 +1098,14 @@ mod tests {
}

// create extra files
file_path = file_path.with_extension("tsconfig.json");
File::create(&file_path).unwrap();
file_path = file_path.with_extension("lock.json");
Copy link
Member Author

Choose a reason for hiding this comment

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

Bug was also happening here where it would do echo_test.tsconfig.json then echo_test.tsconfig.lock.json, which is why this test didn't catch the bug.

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, nice tests!

@dsherret dsherret merged commit d6fd171 into denoland:main Nov 15, 2022
@dsherret dsherret deleted the fix_support_npm_specifiers_deno_install branch November 15, 2022 03:40
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.

None yet

2 participants