Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#### :bug: Bug fix

- Reanalyze: fix reactive/server stale results when cross-file references change without changing dead declarations (non-transitive mode). https://github.com/rescript-lang/rescript/pull/8173
- Add duplicate package detection to rewatch. https://github.com/rescript-lang/rescript/pull/8180

#### :memo: Documentation

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);

eprintln!(
"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
42 changes: 13 additions & 29 deletions tests/build_tests/duplicated_symlinked_packages/input.js
Original file line number Diff line number Diff line change
@@ -1,43 +1,27 @@
// @ts-check

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

const { execBuildLegacy, execCleanLegacy } = setup(import.meta.dirname);
// Set runtime path for rewatch to find
process.env.RESCRIPT_RUNTIME = runtimePath;

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 { stderr } = await execBuild();

const expectedWarning =
"Duplicated package: z ./node_modules/z (chosen) vs ./a/node_modules/z in ./a";
Copy link
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
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" }),
if (!stderr.includes(expectedWarning)) {
assert.fail(
`Expected duplicate package warning not found in stderr.\nExpected: ${expectedWarning}\nActual stderr:\n${stderr}`,
);
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