Skip to content

Commit

Permalink
fix(install): store tags associated with package in node_modules dir (#…
Browse files Browse the repository at this point in the history
…26000)

Fixes #25998. Fixes #25928.

Originally I was just going to make this an error message instead of a
panic, but once I got to a minimal repro I felt that this really should
work.

The panic occurs when you have `nodeModulesDir: manual` (or a
package.json present), and you have an npm package with a tag in your
deno.json (see the spec test that illustrates this).

This code path only actually executes when trying to choose an
appropriate package version from `node_modules/.deno`, so we should be
able to fix it by storing some extra data at install time.

The fix proposed here is to repurpose the `.initialized` file that we
store in `node_modules` to store the tags associated with a package.
Basically, if you have a version requirement with a tag (e.g.
`npm:chalk@latest`), when we set up the node_modules folder for that
package, we store the tag (`latest`) in `.initialized`. Then, when doing
BYONM resolution, if we have a version requirement with a tag, we read
that file and check if the tag is present.

The downside is that we do more work when setting up `node_modules`. We
_could_ do this only when BYONM is enabled, but that would have the
downside of needing to re-run `deno install` when you switch from auto
-> manual, though maybe that's not a big deal.
  • Loading branch information
nathanwhit authored Oct 3, 2024
1 parent 1e0c9b8 commit 2754184
Show file tree
Hide file tree
Showing 8 changed files with 109 additions and 3 deletions.
33 changes: 31 additions & 2 deletions cli/npm/managed/resolvers/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,14 @@ async fn sync_resolution_with_fs(
},
);
let packages_with_deprecation_warnings = Arc::new(Mutex::new(Vec::new()));

let mut package_tags: HashMap<&PackageNv, Vec<&str>> = HashMap::new();
for (package_req, package_nv) in snapshot.package_reqs() {
if let Some(tag) = package_req.version_req.tag() {
package_tags.entry(package_nv).or_default().push(tag);
}
}

for package in &package_partitions.packages {
if let Some(current_pkg) =
newest_packages_by_name.get_mut(&package.id.nv.name)
Expand All @@ -357,11 +365,29 @@ async fn sync_resolution_with_fs(
let package_folder_name =
get_package_folder_id_folder_name(&package.get_package_cache_folder_id());
let folder_path = deno_local_registry_dir.join(&package_folder_name);
let tags = package_tags
.get(&package.id.nv)
.map(|tags| tags.join(","))
.unwrap_or_default();
enum PackageFolderState {
UpToDate,
Uninitialized,
TagsOutdated,
}
let initialized_file = folder_path.join(".initialized");
let package_state = std::fs::read_to_string(&initialized_file)
.map(|s| {
if s != tags {
PackageFolderState::TagsOutdated
} else {
PackageFolderState::UpToDate
}
})
.unwrap_or(PackageFolderState::Uninitialized);
if !cache
.cache_setting()
.should_use_for_npm_package(&package.id.nv.name)
|| !initialized_file.exists()
|| matches!(package_state, PackageFolderState::Uninitialized)
{
// cache bust the dep from the dep setup cache so the symlinks
// are forced to be recreated
Expand All @@ -371,6 +397,7 @@ async fn sync_resolution_with_fs(
let bin_entries_to_setup = bin_entries.clone();
let packages_with_deprecation_warnings =
packages_with_deprecation_warnings.clone();

cache_futures.push(async move {
tarball_cache
.ensure_package(&package.id.nv, &package.dist)
Expand All @@ -389,7 +416,7 @@ async fn sync_resolution_with_fs(
move || {
clone_dir_recursive(&cache_folder, &package_path)?;
// write out a file that indicates this folder has been initialized
fs::write(initialized_file, "")?;
fs::write(initialized_file, tags)?;

Ok::<_, AnyError>(())
}
Expand All @@ -410,6 +437,8 @@ async fn sync_resolution_with_fs(
drop(pb_guard); // explicit for clarity
Ok::<_, AnyError>(())
});
} else if matches!(package_state, PackageFolderState::TagsOutdated) {
fs::write(initialized_file, tags)?;
}

let sub_node_modules = folder_path.join("node_modules");
Expand Down
19 changes: 18 additions & 1 deletion resolvers/deno/npm/byonm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,24 @@ impl<Fs: DenoResolverFs> ByonmNpmResolver<Fs> {
let Ok(version) = Version::parse_from_npm(version) else {
continue;
};
if req.version_req.matches(&version) {
if let Some(tag) = req.version_req.tag() {
let initialized_file =
node_modules_deno_dir.join(&entry.name).join(".initialized");
let Ok(contents) = self.fs.read_to_string_lossy(&initialized_file)
else {
continue;
};
let mut tags = contents.split(',').map(str::trim);
if tags.any(|t| t == tag) {
if let Some((best_version_version, _)) = &best_version {
if version > *best_version_version {
best_version = Some((version, entry.name));
}
} else {
best_version = Some((version, entry.name));
}
}
} else if req.version_req.matches(&version) {
if let Some((best_version_version, _)) = &best_version {
if version > *best_version_version {
best_version = Some((version, entry.name));
Expand Down
44 changes: 44 additions & 0 deletions tests/specs/install/byonm_run_tag_after_install/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"tempDir": true,

"tests": {
"tag_with_byonm": {
"steps": [
{
"args": "install",
"output": "[WILDCARD]"
},
{
"args": "run -A main.ts",
"output": ""
}
]
},
"no_tag_then_tag": {
"steps": [
{
"args": "run -A replace-version-req.ts 1.0.0",
"output": ""
},
{
"args": "install",
"output": "[WILDCARD]"
},
{
"args": "run -A replace-version-req.ts latest",
"output": ""
},
{
"args": "run -A main.ts",
"output": "node_modules_out_of_date.out",
"exitCode": 1
},
{
"args": "install",
"output": "[WILDCARD]"
},
{ "args": "run -A main.ts", "output": "" }
]
}
}
}
5 changes: 5 additions & 0 deletions tests/specs/install/byonm_run_tag_after_install/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"imports": {
"@denotest/esm-basic": "npm:@denotest/esm-basic@latest"
}
}
1 change: 1 addition & 0 deletions tests/specs/install/byonm_run_tag_after_install/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import { add } from "@denotest/esm-basic";
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
error: Could not find a matching package for 'npm:@denotest/esm-basic@latest' in the node_modules directory. Ensure you have all your JSR and npm dependencies listed in your deno.json or package.json, then run `deno install`. Alternatively, turn on auto-install by specifying `"nodeModulesDir": "auto"` in your deno.json file.
at [WILDCARD]main.ts:1:21
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const newReq = Deno.args[0]?.trim();
if (!newReq) {
throw new Error("Missing required argument");
}
const config = JSON.parse(Deno.readTextFileSync("deno.json"));
config.imports["@denotest/esm-basic"] = `npm:@denotest/esm-basic@${newReq}`;
Deno.writeTextFileSync("deno.json", JSON.stringify(config));

0 comments on commit 2754184

Please sign in to comment.