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

fix: support workspace:^ and workspace:~ version constraints #27096

Merged

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Nov 26, 2024

This commit adds support for understanding "workpace:^"
and "workspace:~" version constraints in npm/pnpm workspaces.

This is done by upgrading various crates to their latest versions.

Closes #26726

Comment on lines -280 to -284
/// Resolve the package.json's dependencies.
// TODO(nathanwhit): Remove once we update deno_package_json with dev deps split out
fn resolve_local_package_json_deps(
package_json: &PackageJsonRc,
) -> PackageJsonDeps {
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced with code from deno_config

cli/module_loader.rs Outdated Show resolved Hide resolved
Comment on lines +1 to +10
{
"tempDir": true,
"steps": [{
"args": "install",
"output": "[WILDCARD]"
}, {
"args": "run e/main.ts",
"output": "main.out"
}]
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Test for the actual fix

@bartlomieju bartlomieju marked this pull request as ready for review November 27, 2024 00:09
@bartlomieju bartlomieju requested a review from dsherret November 27, 2024 00:09
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Still looking. Just blocking merge.

cli/args/deno_json.rs Outdated Show resolved Hide resolved
PackageJsonDepWorkspaceReq::Tilde => {
VersionReq::from_raw_text_and_inner(
"workspace:~".to_string(),
RangeSetOrTag::Tag("workspace".to_string()),
Copy link
Member

Choose a reason for hiding this comment

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

I think these should go to a version requirement that's *?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying that this should be "unbounded version range"?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I believe they match any workspace package with the same name.

cli/module_loader.rs Outdated Show resolved Hide resolved
version_req
}
// TODO(bartlomieju): this is not correct, but doesn't really matter until we
// start supporting publishing of npm packages
Copy link
Member

Choose a reason for hiding this comment

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

I think it matters here because this is for publishing. I think also JSR might error because of the * version requirement? Not sure. We should look into fixing this in this PR.

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 thought JSR can't publish based on package.json and these version constraints can only be specified in package.json.

Copy link
Member

@dsherret dsherret Nov 28, 2024

Choose a reason for hiding this comment

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

In this case, it's currently saying that it can depend on any version of the workspace package, but the original workspace dependency is constrained to either ~ or ^

ext/fs/ops.rs Outdated Show resolved Hide resolved
ext/node/ops/require.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit 1d49b3c into denoland:main Nov 29, 2024
17 checks passed
@bartlomieju bartlomieju deleted the chore_update_config_package_json branch November 29, 2024 23:54
bartlomieju added a commit that referenced this pull request Dec 5, 2024
)

This commit adds support for understanding "workpace:^"
and "workspace:~" version constraints in npm/pnpm workspaces.

This is done by upgrading various crates to their latest versions.

Closes #26726

---------

Co-authored-by: David Sherret <dsherret@gmail.com>
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.

Support other workspace: specifiers
3 participants