Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

#### :bug: Bug fix

- Add duplicate package detection to rewatch. https://github.com/rescript-lang/rescript/pull/8180

#### :memo: Documentation

#### :nail_care: Polish
Expand Down
34 changes: 34 additions & 0 deletions rewatch/src/build/packages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ pub fn read_dependency(
/// registered for the parent packages. Especially relevant for peerDependencies.
/// 2. In parallel performs IO to read the dependencies config and
/// recursively continues operation for their dependencies as well.
/// 3. Detects and warns about duplicate packages (same name, different paths).
fn read_dependencies(
registered_dependencies_set: &mut AHashSet<String>,
project_context: &ProjectContext,
Expand All @@ -302,6 +303,37 @@ fn read_dependencies(
.iter()
.filter_map(|package_name| {
if registered_dependencies_set.contains(package_name) {
// Package already registered - check for duplicate (different path)
// Re-resolve from current package and from root to compare paths
if let Ok(current_path) = read_dependency(package_name, package_config, project_context)
&& let Ok(chosen_path) = read_dependency(package_name, &project_context.current_config, project_context)
&& current_path != chosen_path
{
// Different paths - this is a duplicate
let root_path = project_context.get_root_path();
let chosen_relative = chosen_path
.strip_prefix(root_path)
.unwrap_or(&chosen_path);
let duplicate_relative = current_path
.strip_prefix(root_path)
.unwrap_or(&current_path);
let current_package_path = package_config
.path
.parent()
.map(|p| p.to_path_buf())
.unwrap_or_else(|| PathBuf::from("."));
let parent_relative = current_package_path
.strip_prefix(root_path)
.unwrap_or(&current_package_path);

println!(
Comment thread
nojaf marked this conversation as resolved.
Outdated
"Duplicated package: {} ./{} (chosen) vs ./{} in ./{}",
package_name,
chosen_relative.to_string_lossy(),
duplicate_relative.to_string_lossy(),
parent_relative.to_string_lossy()
);
}
None
} else {
registered_dependencies_set.insert(package_name.to_owned());
Expand Down Expand Up @@ -481,6 +513,7 @@ This inconsistency will cause issues with package resolution.\n",
fn read_packages(project_context: &ProjectContext, show_progress: bool) -> Result<AHashMap<String, Package>> {
// Store all packages and completely deduplicate them
let mut map: AHashMap<String, Package> = AHashMap::new();

let current_package = {
let config = &project_context.current_config;
let folder = config
Expand All @@ -500,6 +533,7 @@ fn read_packages(project_context: &ProjectContext, show_progress: bool) -> Resul
show_progress,
/* is local dep */ true,
));

dependencies.iter().for_each(|d| {
if !map.contains_key(&d.name) {
let package = make_package(d.config.to_owned(), &d.path, false, d.is_local_dep);
Expand Down
41 changes: 11 additions & 30 deletions tests/build_tests/duplicated_symlinked_packages/input.js
Original file line number Diff line number Diff line change
@@ -1,43 +1,24 @@
// @ts-check

import * as fs from "node:fs/promises";
import * as assert from "node:assert";
import { setup } from "#dev/process";

const { execBuildLegacy, execCleanLegacy } = setup(import.meta.dirname);

const expectedFilePath = "./out.expected";

const updateTests = process.argv[2] === "update";

/**
* @param {string} output
* @return {string}
*/
function postProcessErrorOutput(output) {
return output.trimEnd().replace(new RegExp(import.meta.dirname, "gi"), ".");
}
const { execBuild, execClean } = setup(import.meta.dirname);

if (process.platform === "win32") {
console.log("Skipping test on Windows");
process.exit(0);
}

await execCleanLegacy();
const { stderr } = await execBuildLegacy();
await execClean();
const { stdout, stderr } = await execBuild();

const expectedWarning =
"Duplicated package: z ./node_modules/z (chosen) vs ./a/node_modules/z in ./a";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The check in bsb was taking symlinks into account according to https://github.com/rescript-lang/rescript/blob/86814cea6e8dfd9dbf756a70a2e6c4964a7c5a60/tests/build_tests/duplicated_symlinked_packages/README.md.

Is the check here also doing that? I.e., is it only warning about z or about other packages, too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I believe so, as we use canonicalize()


const actualErrorOutput = postProcessErrorOutput(stderr.toString());
if (updateTests) {
await fs.writeFile(expectedFilePath, actualErrorOutput);
} else {
const expectedErrorOutput = postProcessErrorOutput(
await fs.readFile(expectedFilePath, { encoding: "utf-8" }),
const output = stdout + stderr;
if (!output.includes(expectedWarning)) {
assert.fail(
`Expected duplicate package warning not found in output.\nExpected: ${expectedWarning}\nActual output:\n${output}`,
);
if (expectedErrorOutput !== actualErrorOutput) {
console.error(`The old and new error output aren't the same`);
console.error("\n=== Old:");
console.error(expectedErrorOutput);
console.error("\n=== New:");
console.error(actualErrorOutput);
process.exit(1);
}
}
2 changes: 0 additions & 2 deletions tests/build_tests/duplicated_symlinked_packages/out.expected

This file was deleted.

Loading