Skip to content

Move is_local_dep to Dependency struct #7736

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 6 commits into from
Jul 30, 2025

Conversation

nojaf
Copy link
Member

@nojaf nojaf commented Jul 29, 2025

I'm in a very confused state writing this, but this PR might hold an answer to rescript-lang/rewatch#124 (comment)

I could be wrong, but I don't think it makes sense to compile the devDependencies of your own non-local dependencies.

In the sample of https://github.com/greenfinity/rescript-sample-v12, a local package has:
"bs-dependencies": ["rescript-nodejs"],.
If I were to build that package with --dev, I don't think we want to build any devDependencies of rescript-nodejs. Like, why would we? For all we know, the sources are missing as in the node_modules anyway.

@jfrolich, @Bushuo, @cknitt any thoughts?

Copy link

pkg-pr-new bot commented Jul 29, 2025

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

commit: 914a870

@cknitt
Copy link
Member

cknitt commented Jul 29, 2025

Agreed, --dev should only build the dev dependencies of local dependencies.

@Bushuo
Copy link
Collaborator

Bushuo commented Jul 29, 2025

Yes, I agree

Copy link
Collaborator

@Bushuo Bushuo left a comment

Choose a reason for hiding this comment

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

Looks good 🔥

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Great work! 🎉

@nojaf nojaf enabled auto-merge (squash) July 30, 2025 08:32
@nojaf nojaf merged commit e4b6669 into rescript-lang:master Jul 30, 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.

4 participants