Skip to content

Fix sourcedirs.json generation #7671

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

Merged
merged 11 commits into from
Jul 21, 2025
Merged

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Jul 19, 2025

Fixes #7670

While testing this locally, I did notice I needed to add a bit more logic to ensure a symbolic linked package in the node_modules is still considered as non local package.

Could already use some eyes on this!

@nojaf nojaf changed the title Generate sourcedirs.json Fix sourcedirs.json generation Jul 19, 2025
Copy link

pkg-pr-new bot commented Jul 19, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7671

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7671

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7671

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7671

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7671

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7671

commit: 27f34bc

@nojaf nojaf marked this pull request as ready for review July 19, 2025 11:52
@@ -455,6 +461,11 @@ This inconsistency will cause issues with package resolution.\n",
);
}

let is_local_dep = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a simpler fix for the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

@@ -65,7 +66,7 @@ pub fn print(buildstate: &BuildState) {
let (dirs, pkgs): (Vec<AHashSet<Dir>>, Vec<AHashMap<PackageName, AbsolutePath>>) = buildstate
.packages
.par_iter()
.filter(|(_name, package)| !package.is_root)
.filter(|(_name, package)| package.is_local_dep && package.source_files.is_some())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case there is only a single rescript.json, this will also ensure it gets processed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal here is to find all packages except for the root package, why is this behavior changing?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh the root is never a local dep, and we don't want to have any other deps? Maybe you should update the comment if people run into this code later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When scaffolding a new (single) rescript project:

Image

it is both root and local, which is fine.
You only need to avoid the root package if it has no source files. So reanalyze can run on it.

@nojaf nojaf requested a review from jfrolich July 20, 2025 08:57
# Conflicts:
#	rewatch/src/sourcedirs.rs
@nojaf nojaf force-pushed the rewatch-sourcedirs branch from 64e3c2d to 9e1a059 Compare July 21, 2025 07:08
@nojaf nojaf merged commit 789591a into rescript-lang:master Jul 21, 2025
27 checks passed
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.

--create-sourcedirs fails with symlink package
2 participants