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 workspace and better Deno workspace support #24334

Merged
merged 101 commits into from
Jul 4, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Jun 25, 2024

Adds much better support for the unstable Deno workspaces as well as support for npm workspaces. npm workspaces is still lacking in that we only install packages into the root node_modules folder. We'll make it smarter over time in order for it to figure out when to add node_modules folders within packages.

A root deno.json file can now specify workspace members like so:

{
  "workspace": ["./member-a", "./member-b"]
}

...these members are folders which can contain a deno.json, package.json, or both. They may or may not be a package. The root deno.json can also be a package itself by specifying "name", "version", and "exports". There are a lot of details here that will be laid out in documentation soon.

This includes a breaking change in config file resolution where we stop searching for config files on the first found package.json unless it's in a workspace. For the previous behaviour, the root deno.json needs to be updated to be a workspace by adding "workspace": ["./path-to-pkg-json-folder-goes-here"]. See details in denoland/deno_config#66

Note: this is missing wildcard support for npm workspaces (follow #24420 for updates)

Part of #22942

Closes #24340
Closes #24159
Closes #24161
Closes #22020
Closes #18546
Closes #16106
Closes #24160

@dsherret dsherret changed the title feat: better workspace support feat: npm workspace and better Deno workspace support Jul 3, 2024
@dsherret dsherret marked this pull request as ready for review July 3, 2024 17:09
- Get all node_modules in dir for deno compile w/ byonm
- Maybe fix Windows CI, but probably won't fix
Comment on lines -928 to -931
if *DENO_FUTURE {
Some(current_dir.to_path_buf())
} else {
None
Copy link
Member

Choose a reason for hiding this comment

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

What are implications of removing this condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

deno_config just always resolved the Workspace now. That conditionally resolves the package.json based on whether package_json_discovery in the discover options is true. Right now, it only doesn't discover the package.json if --no-npm or --no-config is provided. I'm not sure it's worth making it more granular than that because all the perf sensitive commands need it anyway.

| Compile(CompileFlags {
source_file: script,
..
}) => {
Copy link
Member Author

@dsherret dsherret Jul 3, 2024

Choose a reason for hiding this comment

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

Note this change. We should be discovering the workspace based on the script provided to deno compile. Previously we were always using the cwd.

cli/args/mod.rs Outdated Show resolved Hide resolved
cli/args/mod.rs Outdated Show resolved Hide resolved
Comment on lines +1112 to +1114
// todo(dsherret): this should be false for nodeModulesDir: true
pkg_json_dep_resolution: if self.use_byonm() {
PackageJsonDepResolution::Disabled
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to address that before landing?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's a big change.

.as_ref()
.map(|c| c.has_unstable("sloppy-imports"))
.unwrap_or(false)
|| self.workspace.has_unstable("sloppy-imports")
Copy link
Member

Choose a reason for hiding this comment

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

I must have missed it, but how are unstable features enabled in the workspace setting? I hope it's only at the top level?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's only at the top level. Otherwise it surfaces diagnostics in deno_config.

cli/args/package_json.rs Outdated Show resolved Hide resolved
cli/standalone/binary.rs Outdated Show resolved Hide resolved
cli/tools/registry/unfurl.rs Outdated Show resolved Hide resolved
.unwrap_or_default();
let start_ctx = cli_options.workspace.resolve_start_ctx();
if !start_ctx.has_deno_or_pkg_json() {
bail!("deno task couldn't find deno.json(c). See https://deno.land/manual@v{}/getting_started/configuration_file", env!("CARGO_PKG_VERSION"))
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 mention that we couldn't find package.json either?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to encourage people to use a deno.json (this message is the same as before)

@@ -48,10 +48,17 @@ pub async fn vendor(
validate_options(&mut cli_options, &output_dir)?;
let factory = CliFactory::from_cli_options(Arc::new(cli_options));
let cli_options = factory.cli_options();
if cli_options.workspace.config_folders().len() > 1 {
bail!("deno vendor is not supported in a workspace. Set `\"vendor\": true` in the workspace deno.json file instead");
}
Copy link
Member

Choose a reason for hiding this comment

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

Agreed

Comment on lines +166 to +168
if paths.is_empty() {
return Ok(());
}
Copy link
Member

Choose a reason for hiding this comment

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

What prompted this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it in here instead of being done only sometimes conditionally in the higher level code.

@dsherret dsherret requested a review from bartlomieju July 3, 2024 23:48
@dsherret dsherret enabled auto-merge (squash) July 3, 2024 23:54
@dsherret dsherret merged commit 147411e into denoland:main Jul 4, 2024
17 checks passed
@dsherret dsherret deleted the workspace_start branch July 4, 2024 01:19
@tungv

This comment was marked as off-topic.

@dsherret

This comment was marked as off-topic.

@tungv

This comment was marked as off-topic.

@dsherret

This comment was marked as off-topic.

@tungv
Copy link

tungv commented Jul 4, 2024

thanks: #24430

zebreus pushed a commit to zebreus/deno that referenced this pull request Jul 8, 2024
Adds much better support for the unstable Deno workspaces as well as
support for npm workspaces. npm workspaces is still lacking in that we
only install packages into the root node_modules folder. We'll make it
smarter over time in order for it to figure out when to add node_modules
folders within packages.

This includes a breaking change in config file resolution where we stop
searching for config files on the first found package.json unless it's
in a workspace. For the previous behaviour, the root deno.json needs to
be updated to be a workspace by adding `"workspace":
["./path-to-pkg-json-folder-goes-here"]`. See details in
denoland/deno_config#66

Closes denoland#24340
Closes denoland#24159
Closes denoland#24161
Closes denoland#22020
Closes denoland#18546
Closes denoland#16106
Closes denoland#24160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment