Skip to content
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

Use Yarn workspace #7357

Merged
merged 13 commits into from
Mar 24, 2025
Merged

Use Yarn workspace #7357

merged 13 commits into from
Mar 24, 2025

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Mar 22, 2025

Solves #7298

Experiment using Yarn workspace instead of #7356

Yarn's advantage:

  • Faster
  • Cleaner
  • Secure
  • Sensible, deterministic behavior
  • More built-in utilities, e.g:
    • Topological/Recursive command executor
    • Constraints for workspace
    • yarn up @rescript/react "@biomejs/*"

@cometkim cometkim force-pushed the yarn-workspace branch 3 times, most recently from 52f097c to a098fe1 Compare March 22, 2025 23:04
@cometkim
Copy link
Member Author

cometkim commented Mar 22, 2025

The "workspace:" protocol helped to create rescript link in the nested node_modules path.

@cometkim cometkim mentioned this pull request Mar 22, 2025
@cometkim cometkim requested a review from cknitt March 22, 2025 23:36
@cometkim cometkim marked this pull request as ready for review March 22, 2025 23:37
@cknitt
Copy link
Member

cknitt commented Mar 23, 2025

Personally I like this a lot (big fan of yarn and using it in all our company projects).

Not sure how people not usually using yarn feel about it though. I see you tried the same with npm in #7356, but it was not feasible?

The contributing guide would also need to be updated for yarn.

@cometkim
Copy link
Member Author

cometkim commented Mar 23, 2025

Not sure how people not usually using yarn feel about it though. I see you tried the same with npm in #7356, but it was not feasible?

I tried npm first, but the difference in how workspace dependencies are linked had an unintended impact on the docstring tests.

npm doesn't provide any functionality for workspaces other than basic installation, so there is no workaround.

yarn provides more options here, such as workspace: and portal:. It is simple to configure, except for installing yarn CLI, and provides a very stable lockfile format.

@cometkim cometkim force-pushed the yarn-workspace branch 2 times, most recently from 529e468 to f2f52c3 Compare March 23, 2025 12:38
@cometkim
Copy link
Member Author

cometkim commented Mar 23, 2025

Don't get it. How is it fixed if these completions are still missing?

@cknitt I deleted it intentionally from 93f16ca

Because we don't need the entire API surface to verify the behavior.

@cknitt
Copy link
Member

cknitt commented Mar 23, 2025

@cknitt I deleted it intentionally from 93f16ca

Ah, got it! 🙂

@cknitt
Copy link
Member

cknitt commented Mar 24, 2025

Could you also add a CHANGELOG entry?

@cometkim
Copy link
Member Author

Could you also add a CHANGELOG entry?

Done.

Honestly, I'm not sure we should keep verbose logs for not user-facing changes. We do have context in PRs anyway

@cknitt
Copy link
Member

cknitt commented Mar 24, 2025

Thanks!

Honestly, I'm not sure we should keep verbose logs for not user-facing changes. We do have context in PRs anyway

Certainly not for the small stuff, but I thought this one was a big enough internal change to warrant putting it into the CHANGELOG.

@cometkim cometkim merged commit 7e7f138 into rescript-lang:master Mar 24, 2025
20 checks passed
@cometkim cometkim deleted the yarn-workspace branch March 24, 2025 19:51
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.

2 participants