ReScript cli clean up + watcher fix #6404

DZakh · 2023-09-14T23:36:33Z

Fixed dependencies reinitialization on every save in watch mode.

Before:

After:

Also:

Actualized help for commands
Started showing the compile time for rescript build command without watch mode
Pass through the -verbose flag to builds in watch mode

What I want to do next:

Fix the build needed condition.
Resolve the problems with duplicated builds (existed before the changes)
Enable watch mode for pinned dependencies 🤔
Update documentation

DZakh · 2023-09-14T23:42:39Z

jscomp/bsb_exe/rescript_main.ml

@@ -26,7 +26,7 @@ let () = Bsb_log.setup ()

 let separator = "--"

-let watch_mode = ref false


I've separated the watch mode from other delegated commands to handle it separately. So now we have better control over it, and the ref is unnecessary.

Before:

Trigger build and exit with 1

Start watching changes

Trigger build

After:

Trigger build

Start watching changes

DZakh · 2023-09-14T23:44:54Z

jscomp/bsb_exe/rescript_main.ml

@@ -26,7 +26,7 @@ let () = Bsb_log.setup ()

 let separator = "--"

-let watch_mode = ref false
+let no_deps_mode = ref false


The first build should be done with dependencies. Builds on change should be without dependencies.

DZakh · 2023-09-14T23:45:36Z

jscomp/bsb_exe/rescript_main.ml

@@ -84,13 +84,13 @@ let ninja_command_exit (type t) (ninja_args : string array) : t =
   ninja -C _build
 *)
 let clean_usage =
-  "Usage: rescript.exe clean <options>\n\n\
-   `rescript clean` only cleans the current project\n"
+  "Usage: rescript clean <options>\n\n\


Removed exe and updated the description. It's more relevant now, since the -with-deps is deprecated.

DZakh · 2023-09-14T23:46:11Z

jscomp/bsb_exe/rescript_main.ml

+      ("-w", unit_set_spec (ref false), "Watch mode");
+      ( "-ws",
+        string_set_spec (ref ""),
+        "[host]:port set up host & port for WebSocket build notifications" );
+      ("-verbose", call_spec Bsb_log.verbose, "Set the output to be verbose");


Sorted in the order:

Public

Deprecated

Internal

DZakh · 2023-09-14T23:46:56Z

jscomp/bsb_exe/rescript_main.ml

      ( "-regen",
        unit_set_spec force_regenerate,
        "*internal* \n\
         Always regenerate build.ninja no matter bsconfig.json is changed or \
         not" );
-      ("-verbose", call_spec Bsb_log.verbose, "Set the output to be verbose");
+      ("-no-deps", unit_set_spec no_deps_mode, "*internal* Needed for watcher to build without dependencies on file change");


I don't see a usecase for it, so I've added the flag as internal

DZakh · 2023-09-14T23:53:52Z

rescript

+      .listen(webSocketPort, webSocketHost);
+  }
+
+  function watchBuild(watchConfig) {


This function is not touched. Only the watchGenerated definition moved to a higher scope.

DZakh · 2023-09-14T23:55:36Z

rescript

+    return !(
+      fileName === ".merlin" ||
+      fileName.endsWith(".js") ||
+      fileName.endsWith(".mjs") ||
+      fileName.endsWith(".cjs") ||
+      fileName.endsWith(genTypeFileExtension) ||
+      watchGenerated.indexOf(fileName) >= 0 ||
+      fileName.endsWith(".swp")
+    );


I have a big question about the logic. Why not check only .res,.resi,.ml,.mli changes?
Also, since the whole condition is negated the watchGenerated.indexOf(fileName) >= 0 doesn't trigger rebuild on generated file change 😅

For example, if ReScript is used in a codebase mixed with ts. It'll recompile on every ts file change

Good question. Maybe @bobzhang can comment?

DZakh · 2023-09-14T23:55:59Z

rescript

-        if (postBuild) {
-          dlog(`running postbuild command: ${postBuild}`);
-          child_process.exec(postBuild);
-        }


DZakh · 2023-09-14T23:57:27Z

rescript

+  logStartCompiling();
+  delegateCommand(delegatedArgs, () => {
+    startWatchMode(withWebSocket);
+    buildFinishedCallback(0);


This line initializes file watchers.

DZakh · 2023-09-14T23:58:00Z

rescript

+    case "info":
+    case "clean": {
+      delegateCommand(process_argv.slice(2));
+      break;
+    }
+    case undefined:
+    case "build": {
+      logStartCompiling();
+      delegateCommand(process_argv.slice(2), logFinishCompiling);
+      break;
+    }


This way, we can separate the watcher-specific code.

DZakh · 2023-09-19T06:23:12Z

The PR is ready for review

cristianoc · 2023-09-19T07:24:36Z

@cknitt up for a review?

cknitt · 2023-09-19T15:09:27Z

Great work @DZakh! I'll test it tomorrow against our current project.

cknitt · 2023-09-19T15:13:09Z

jscomp/bsb_exe/rescript_main.ml

-  "Usage: rescript.exe clean <options>\n\n\
-   `rescript clean` only cleans the current project\n"
+  "Usage: rescript clean <options>\n\n\
+   `rescript clean` cleans build artifacts. It is useful for edge-case reasons in case you get into a stale build\n"


Not sure about the wording here or if we should mention edge cases/stale builds at all.

Maybe just

Suggested change

`rescript clean` cleans build artifacts. It is useful for edge-case reasons in case you get into a stale build\n"

`rescript clean` cleans build artifacts\n"

?

What do you think @cristianoc?

That's cleaner

Ok, then could you accept my change suggestion @DZakh?

cknitt · 2023-09-20T06:25:12Z

Great work @DZakh! I'll test it tomorrow against our current project.

Ok, our current project is a monorepo project with its own watcher, so I wasn't able to test the watch mode changes against it.

I tested briefly against an example project and everything seemed fine though.

DZakh · 2023-09-20T07:47:55Z

Done 👌

CHANGELOG.md

DZakh · 2023-09-21T19:16:13Z

I think it's ready to be merged

zth · 2023-09-26T20:30:07Z

@DZakh seems we missed our window and there's now a few conflicts. Care fixing and we can merge after?

Co-authored-by: Christoph Knittel <christoph@knittel.cc>

DZakh · 2023-09-27T17:04:09Z

Done

DZakh commented Sep 14, 2023

View reviewed changes

cknitt reviewed Sep 19, 2023

View reviewed changes

DZakh force-pushed the watch-mode-clean-up branch from 63a5c58 to 40a81bc Compare September 20, 2023 07:47

cknitt reviewed Sep 20, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cknitt approved these changes Sep 20, 2023

View reviewed changes

DZakh and others added 4 commits September 27, 2023 20:03

Watch mode clean up

56b7649

Update clean command help

4d1f7fb

Update changelog

56c464b

Update CHANGELOG.md

70b514a

Co-authored-by: Christoph Knittel <christoph@knittel.cc>

DZakh force-pushed the watch-mode-clean-up branch from 4a0890b to 70b514a Compare September 27, 2023 17:03

DZakh requested a review from cristianoc September 27, 2023 17:04

zth merged commit 7e2e81f into rescript-lang:master Sep 27, 2023
7 checks passed

fhammerschmidt mentioned this pull request Oct 2, 2023

Document v11 features. rescript-association/rescript-lang.org#697

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReScript cli clean up + watcher fix #6404

ReScript cli clean up + watcher fix #6404

DZakh commented Sep 14, 2023 •

edited

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 15, 2023

cknitt Sep 20, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh Sep 14, 2023

DZakh commented Sep 19, 2023

cristianoc commented Sep 19, 2023

cknitt commented Sep 19, 2023

cknitt Sep 19, 2023

cristianoc Sep 19, 2023

cknitt Sep 20, 2023

cknitt commented Sep 20, 2023 •

edited

DZakh commented Sep 20, 2023

DZakh commented Sep 21, 2023

zth commented Sep 26, 2023

DZakh commented Sep 27, 2023

		@@ -26,7 +26,7 @@ let () = Bsb_log.setup ()

		let separator = "--"

		let watch_mode = ref false

	`rescript clean` cleans build artifacts. It is useful for edge-case reasons in case you get into a stale build\n"
	`rescript clean` cleans build artifacts\n"

ReScript cli clean up + watcher fix #6404

ReScript cli clean up + watcher fix #6404

Conversation

DZakh commented Sep 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DZakh commented Sep 19, 2023

cristianoc commented Sep 19, 2023

cknitt commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cknitt commented Sep 20, 2023 • edited

DZakh commented Sep 20, 2023

DZakh commented Sep 21, 2023

zth commented Sep 26, 2023

DZakh commented Sep 27, 2023

DZakh commented Sep 14, 2023 •

edited

cknitt commented Sep 20, 2023 •

edited