Be more aware of the workspace during clean #7752

nojaf · 2025-08-02T20:31:11Z

This is an attempt to drive the conversation at #7707

In case of clean, we already do an upward traversal to find a root rescript json.
I'm adding some logic to check if the parent rescript json contains the current rescript json in "dependencies" (or "dev-dependencies").

If we did find a link, we would only remove the files from the current project and not everything.

Consider https://github.com/nojaf/rescript-kaplay/blob/main/packages/rescript-kaplay/rescript.json

We already find https://github.com/nojaf/rescript-kaplay/blob/d38649fcd409c174c39b31488f790e036f3b0a20/rescript.json#L8

If that name matches root_package (in the code, a bit confusing name) is "@nojaf/rescript-kaplay", so we only need to remove those files.

Files from https://github.com/nojaf/rescript-kaplay/blob/main/packages/samples/rescript.json are unaffected.

If we were to clean from the repo root, root_config would be the well the root package and everything gets cleaned.

rewatch/src/build/clean.rs

nojaf

Let me know if something doesn't make any sense.

pkg-pr-new · 2025-08-02T20:53:47Z

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

commit: fc66c24

nojaf · 2025-08-04T17:31:50Z

I think it might make sense to rethink the format command as well.

Would it not be more consistent if we format either:

a specific file argument like format Foo.res
Format all projects in case of a monorepo when invoked at the root.
Format a specific project only when it is part of a workspace, but invoked at the project level and not the root.

The --all flag seems a bit off compared to clean.

cknitt · 2025-08-07T06:52:39Z

Agreed, defaulting to all / keeping args similar to build/clean definitely makes sense!

nojaf · 2025-08-07T12:10:39Z

There still is more work to do here.
In general the concept of current_package and workspace_package and their relation should be cemented more in all code paths.

my-monorepo/
  packages/
    a/
      rescript.json
    b/
      rescript.json
rescript.json

Running clean, build, format and compiler-args inside a or my-monorepo need to clearly defined.

	repository (my-monorepo)	package (a)	expectation repository	expectation project
compiler-args	packages/a/src/File.res	src/Files.res	Get compiler args. Extension from repo is used	Exact same result, extension from repo is used
format			All files of all packages are formatted	Files of current package are formatted
build			All packages are built	Only this package is built
clean			All packages are cleaned	Only this packaged is cleaned

I also think that if there is no link with package and repository rescript, workspace_path should be None.

For the time being if there is a workspace rescript.json, that "suffix" is primary.
Nested rescript.json files cannot define it again. Maybe we add a warning for that.

Post v12 we can explore the option to override suffix in child packages, but it is my understanding that it doesn't work that way today.

_{I'm also in favour of renaming root_package of current_package in code.}

nojaf · 2025-08-12T07:43:58Z

rescript.json

@@ -1,4 +1,4 @@
 {
  "name": "rescript",
-  "dependencies": ["@tests/gentype-react-example"]
+  "dependencies": ["@tests/gentype-react-example", "playground"]


"playground" is part of the workspace, so I need to be listed here for rewatch to pick up the correct node_modules folder.

The package.json has rescript clean in the commands.
This will now accurately only clean the playground package.

But it needs that link to the parent rescript json.

nojaf · 2025-08-12T07:46:03Z

rewatch/tests/suite-ci.sh

 bold "Rescript version"
-(cd ../testrepo && ./node_modules/.bin/rescript -v)
+(cd ../testrepo && ./node_modules/.bin/rescript --version)


-v is verbosity and this was trying to compile the testrepo with an older installed version of rescript.
I'm not sure how this wasn't leading to bigger problems.

nojaf · 2025-08-12T07:47:18Z

rewatch/src/project_context.rs

+// I don't think this linting rule matters very much as we only create a single instance of this enum.
+// See https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
+#[allow(clippy::large_enum_variant)]
+pub enum ProjectContext {


This streamlines the many workspace: Option<string> checks that were being done.

It improves the readability and debugability of rewatch.

I would maybe do:

pub enum MonoRepoContext { Root {local_dependencies, local_dev_dependencies} Package {parent_config: Config, parent_path: RescriptJsonPath} } pub struct ProjectContext { config: Config, path: RescriptJsonPath, mono_repo_context: Option<MonoRepoContext> }

I would like to be mindful about memory consumption - even if minor - you never know how much these will grow in the future - and I think we should try and keep a small footprint (smaller than we do now). I think the enum will otherwise always take the biggest size when allocating, even in smaller contexts.

nojaf · 2025-08-12T08:08:34Z

For those overwhelmed by these changes: in short, this resolves the expectations described in #7707 comment.

This PR makes Rewatch more aware of its context:

single project
monorepo with Rewatch run at the root
monorepo with Rewatch run at the package level

In the last case, Rewatch will run build, clean, and format only for that package. This seems sensible.

I removed the --all flag for formatting; formatting now defaults to the current project.

I will add a changelog entry if there is consensus to move forward.

Please ask any questions you may have.
If you read this and don't have any thoughts or feedback, please leave a 👀 on the PR to indicate silent agreement.

Much has changed since this review.

rolandpeelen

Really nice work! Left some comments on the Rust bits and pieces 👌 / some optimisations.

rewatch/src/build/clean.rs

rolandpeelen · 2025-08-12T10:30:55Z

rewatch/src/build/compile.rs

@@ -709,7 +699,7 @@ fn compile_file(
            }

            // copy js file
-            root_package.config.get_package_specs().iter().for_each(|spec| {
+            root_config.get_package_specs().iter().for_each(|spec| {


More of a note to myself for some future refactor - I think we can do better with memory management - the package specs / root_config and those things are essentially constant after reading. We should be able - for the most part - to just always return references rather than cloning - I think we can further smallen the footprint / make things quite a bit faster that way

This function also get's called a lot. I think we might benefit from some more abstraction, having an internal representation for a package / packagespec, rather than the parseTree we need for Serde

5 usages indeed, worth revisiting. Will consider this outside the scope of this PR, as it is already so large.

we pass references right? Where are we doing cloning?

rewatch/src/build/packages.rs

rolandpeelen · 2025-08-12T10:44:02Z

rewatch/src/project_context.rs

+// I don't think this linting rule matters very much as we only create a single instance of this enum.
+// See https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
+#[allow(clippy::large_enum_variant)]
+pub enum ProjectContext {


I would maybe do:

pub enum MonoRepoContext { Root {local_dependencies, local_dev_dependencies} Package {parent_config: Config, parent_path: RescriptJsonPath} } pub struct ProjectContext { config: Config, path: RescriptJsonPath, mono_repo_context: Option<MonoRepoContext> }

I would like to be mindful about memory consumption - even if minor - you never know how much these will grow in the future - and I think we should try and keep a small footprint (smaller than we do now). I think the enum will otherwise always take the biggest size when allocating, even in smaller contexts.

rewatch/src/project_context.rs

rolandpeelen · 2025-08-12T10:52:49Z

rewatch/src/project_context.rs

+        context
+    }
+
+    pub fn get_root_config(&self) -> &Config {


Breaking up the enum into a struct for the shared fields would remove the need for these too, which is nice imo :)

It's not exactly the same, because we want to get different things (in a mono repo package, get the root config, otherwise get the config of the current package). So I think it's pretty nice like this - and with an implementation, it's easy to get the configuration.

Yeah, check you're right actually. Misread - these would still be there :) - still think having the overlapping fields in a struct would model the real world better though

@rolandpeelen I've refactored based on the feedback,

now I have

#[allow(clippy::large_enum_variant)] pub enum ProjectContext { /// Single project - no workspace relationship SingleProject { config: Config }, /// Monorepo root - contains local dependencies (symlinked in node_modules) MonorepoRoot { config: Config, local_dependencies: AHashSet<String>, // names of local deps local_dev_dependencies: AHashSet<String>, }, /// Package within a monorepo - has a parent workspace MonorepoPackage { config: Config, parent_config: Config }, }

How would change that? They all share their own Config. How to continue?

I would personally still break it out to a struct + enum, all have the same config object at the root, and only when you have a monorepo, there are additional fields.

So a project always has a config, but it may have a parent config (in the case of a package within a monorepo), or may have local dependencies (in case it's the root package) - but have no strong opinion.

The functions could still be there, because the 'root' would change in the case of a monorepo-package, but you'd for instance won't need the get_current_rescript_config - it would just be x.config, which makes a lot more sense to me than having an abstraction over a field that exists for all cases.

Come to think of it - I think we can leave it like this - the one property you lose doing this is an easy (and very clear) distinction you can use everywhere, and perhaps that's worth more than this is.

rewatch/src/project_context.rs

jfrolich · 2025-08-12T13:42:09Z

rewatch/src/project_context.rs

+// See https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
+#[allow(clippy::large_enum_variant)]
+pub enum ProjectContext {
+    /// Single project - no workspace relationship


Super nice, I like this a lot!

nojaf · 2025-08-13T12:41:27Z

Okay, I'm going to merge this. I want to get it over it.
Happy to revisit things in follow-up PRs.

I didn't hear any strong objections to the CLI changes. I think this is fine, but if you're reading this after the merge and still want to discuss, please speak up.

nojaf added 2 commits August 2, 2025 21:43

Be more aware of the workspace during clean

776801c

Only clean current package

dff32e7

nojaf commented Aug 2, 2025

View reviewed changes

rewatch/src/build/clean.rs Outdated Show resolved Hide resolved

nojaf commented Aug 2, 2025

View reviewed changes

fmt

119d553

nojaf added 3 commits August 4, 2025 09:30

Return is_child from packages make

78bfd88

Invert if

81966e0

Introduce PackageMap to make things a bit more clear.

7b84cb4

jfrolich approved these changes Aug 7, 2025

View reviewed changes

jfrolich previously approved these changes Aug 7, 2025

View reviewed changes

nojaf added 2 commits August 7, 2025 13:52

Use workspace extension if there is a link, otherwise root_package.

178a0ec

Add a TODO that we need to clean up some confusing bits.

5b2e427

nojaf added 9 commits August 9, 2025 19:07

Refactor using project_context

08a146d

A rescript.json with an unrelated parent can still be a monorepo.

860e6e2

Remove all from format command.

8202315

Merge branch 'master' into detect-root

d29f977

get_rescript_legacy inside rescript repository.

c2c9183

sigh

50baa0b

StrippedVerbatimPath in before local dep check

c19670e

Use proper --version

2b8e412

Add playground to monorepo

43097c8

nojaf requested a review from Copilot August 11, 2025 10:45

This comment was marked as outdated.

Sign in to view

nojaf added 3 commits August 11, 2025 12:57

Copilot nitpicks

c740f4e

Add test for only formatting the current project.

e5b723c

Add clean single project test

7b9b230

nojaf marked this pull request as ready for review August 12, 2025 07:42

nojaf commented Aug 12, 2025

View reviewed changes

nojaf requested a review from jfrolich August 12, 2025 07:48

nojaf linked an issue Aug 12, 2025 that may be closed by this pull request

Rescript clean of individual project #7707

Closed

rolandpeelen reviewed Aug 12, 2025

View reviewed changes

nojaf added 2 commits August 12, 2025 14:50

Address code review remarks of clean.rs

c8ebab8

Store path to config in Config struct

067e983

jfrolich reviewed Aug 12, 2025

View reviewed changes

jfrolich approved these changes Aug 12, 2025

View reviewed changes

nojaf added 14 commits August 12, 2025 15:56

Format

2606104

Add with-ppx example

c463e94

Ensure parse arguments of source package are used.

607b412

Add compiled file from with-ppx

fec8b5d

Update snapshot because new test project.

ccbd698

Update suffix test.

1db7bbe

One more file to format

ad06c8b

Moar code review suggestions.

db6883b

Print if a package is local after checking.

e01de31

Remove debug log

dbe85aa

Merge branch 'master' into detect-root

70718a6

Windows fix of the day

ddb75e3

Wait a bit more on watch file for Windows CI

e59a7f4

Add changelog entry

b014cd1

rolandpeelen approved these changes Aug 13, 2025

View reviewed changes

Extract common data from enum.

fc66c24

nojaf merged commit 2cd9996 into rescript-lang:master Aug 13, 2025
27 checks passed

nojaf mentioned this pull request Aug 13, 2025

Use oxc_resolver for ppx path resolution #7740

Open

Be more aware of the workspace during clean #7752

Be more aware of the workspace during clean #7752

Conversation

nojaf commented Aug 2, 2025

Uh oh!

Uh oh!

nojaf left a comment

Choose a reason for hiding this comment

Uh oh!

pkg-pr-new bot commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojaf commented Aug 4, 2025

Uh oh!

cknitt commented Aug 7, 2025

Uh oh!

nojaf commented Aug 7, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf commented Aug 12, 2025

Uh oh!

rolandpeelen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

pkg-pr-new bot commented Aug 2, 2025 •

edited

Loading