-
Notifications
You must be signed in to change notification settings - Fork 455
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
ReScript cli clean up + watcher fix #6404
Conversation
@@ -26,7 +26,7 @@ let () = Bsb_log.setup () | |||
|
|||
let separator = "--" | |||
|
|||
let watch_mode = ref false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -26,7 +26,7 @@ let () = Bsb_log.setup () | |||
|
|||
let separator = "--" | |||
|
|||
let watch_mode = ref false | |||
let no_deps_mode = ref false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first build should be done with dependencies. Builds on change should be without dependencies.
@@ -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\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed exe and updated the description. It's more relevant now, since the -with-deps
is deprecated.
("-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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted in the order:
- Public
- Deprecated
- Internal
( "-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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a usecase for it, so I've added the flag as internal
.listen(webSocketPort, webSocketHost); | ||
} | ||
|
||
function watchBuild(watchConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not touched. Only the watchGenerated
definition moved to a higher scope.
return !( | ||
fileName === ".merlin" || | ||
fileName.endsWith(".js") || | ||
fileName.endsWith(".mjs") || | ||
fileName.endsWith(".cjs") || | ||
fileName.endsWith(genTypeFileExtension) || | ||
watchGenerated.indexOf(fileName) >= 0 || | ||
fileName.endsWith(".swp") | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if ReScript is used in a codebase mixed with ts. It'll recompile on every ts file change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Maybe @bobzhang can comment?
if (postBuild) { | ||
dlog(`running postbuild command: ${postBuild}`); | ||
child_process.exec(postBuild); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
logStartCompiling(); | ||
delegateCommand(delegatedArgs, () => { | ||
startWatchMode(withWebSocket); | ||
buildFinishedCallback(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line initializes file watchers.
case "info": | ||
case "clean": { | ||
delegateCommand(process_argv.slice(2)); | ||
break; | ||
} | ||
case undefined: | ||
case "build": { | ||
logStartCompiling(); | ||
delegateCommand(process_argv.slice(2), logFinishCompiling); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, we can separate the watcher-specific code.
The PR is ready for review |
@cknitt up for a review? |
Great work @DZakh! I'll test it tomorrow against our current project. |
jscomp/bsb_exe/rescript_main.ml
Outdated
"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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the wording here or if we should mention edge cases/stale builds at all.
Maybe just
`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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then could you accept my change suggestion @DZakh?
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. |
63a5c58
to
40a81bc
Compare
Done 👌 |
I think it's ready to be merged |
@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 <[email protected]>
4a0890b
to
70b514a
Compare
Done |
Fixed dependencies reinitialization on every save in watch mode.
Before:
After:
Also:
rescript build
command without watch mode-verbose
flag to builds in watch modeWhat I want to do next: