-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
- Get all node_modules in dir for deno compile w/ byonm - Maybe fix Windows CI, but probably won't fix
if *DENO_FUTURE { | ||
Some(current_dir.to_path_buf()) | ||
} else { | ||
None |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, | ||
.. | ||
}) => { |
There was a problem hiding this comment.
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.
// todo(dsherret): this should be false for nodeModulesDir: true | ||
pkg_json_dep_resolution: if self.use_byonm() { | ||
PackageJsonDepResolution::Disabled |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
.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")) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
if paths.is_empty() { | ||
return Ok(()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What prompted this change?
There was a problem hiding this comment.
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.
…o_config's test suite
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
thanks: #24430 |
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
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:
...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#66Note: 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