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(unstable): ability to npm install then deno run main.ts #20967

Merged
merged 66 commits into from
Oct 25, 2023

Conversation

dsherret
Copy link
Member

This PR adds a new unstable "bring your own node_modules" (BYONM) functionality currently behind a --unstable-byonm flag ("unstable": ["byonm"] in a deno.json).

This enables users to run a separate install command (ex. npm install, pnpm install) then run deno run main.ts and Deno will respect the layout of the node_modules directory as setup by the separate install command. It also works with npm/yarn/pnpm workspaces.

For this PR, the behaviour is opted into by specifying --unstable-byonm/"unstable": ["byonm"], but in the future we may make this the default behaviour as outlined in #18967 (comment)

This is an extremely rough initial implementation. Errors are terrible in this and the LSP requires frequent restarts. Improvements will be done in follow up PRs.

Part of #18967

kt3k and others added 30 commits September 29, 2023 00:40

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@dsherret dsherret requested a review from bartlomieju October 24, 2023 21:46
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.

Maybe upstream test_util/ changes in a separate PR?

@dsherret dsherret requested a review from bartlomieju October 25, 2023 14:34
if let Some(resolver) =
self.npm_resolver.as_ref().and_then(|r| r.as_byonm())
{
match &result {
Copy link
Member Author

Choose a reason for hiding this comment

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

For the future, this is the point where we need to make the most improvement. Essentially, we need a way to determine if a specifier is indeed for an npm package and then we need to ensure we surface the appropriate error messages when it fails.

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, exciting!

@dsherret dsherret merged commit be97170 into denoland:main Oct 25, 2023
@dsherret dsherret deleted the byonm_part1 branch October 25, 2023 18:39
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

3 participants