feat(unstable): Support --watch flag for bundle and fmt subcommands #8276

magurotuna · 2020-11-07T07:05:44Z

This PR adds --watch flag support for both bundle and fmt subcommands.
In addition to this, it refactors run --watch command so that module resolution will occur every time the file watcher detects file addition/deletion, which allows the watcher to observe a file that is newly added to the dependency as well.

Fixes #8181

Currently, --watch flag is working with run only, but it is considered the flag is going to be added to other subcommands like fmt, lint, and so on. (see #7465)
For this purpose, the current implementation of watch_func function has abstract, extendable design - all we have to do to add the watcher feature to subcommands is passing both target file paths and an operation you want to execute to the watch_func function.

Considering it deeply, it seems like this watch_func design is not suitable for usecases, especially run subcommand. In case we want to detect file changes and then restart it, the watcher should also keep an eye on deletion and addition of files or dependency.
So target file paths will vary, although the current watch_func receives constant file paths. This is why the issue described in #8181 happened.

To address the issue, I think we should resolve dependency whenever the watcher detects changes. Actually this is relatively heavy task to be done every time, but it seems needed.

In this PR, I've added new watcher funtcion named watch_func_for_run which is dedicated to run subcommand.
Additionally I've removed the original watch_func function. I know this removal will have an effect on adding --watch flag to other subcommands, which are being worked on at #7740 and #7767. I think other subcommands, like fmt, also have the same issue as run. That said, if we would run deno fmt --watch, the watcher would have to detect any addition or deletion of files. Therefore we should probably consider again what watch_func function interface should look like, without using the current one.

TODO

add test

magurotuna · 2020-11-10T16:48:54Z

I also added fmt --watch feature to show how we can make subcommands other than run work with --watch flag. (a lot of code come from #7767)

magurotuna · 2020-11-11T16:21:53Z

Just added bundle --watch as well. Bundle is similar to run rather than fmt, so I renamed watch_func_for_run to watch_func_with_module_resolution and fixed its parameter type a little.

bartlomieju · 2020-11-11T16:23:08Z

watch_func_with_module_resolution - how about watch_func_with_module_graph?

magurotuna · 2020-11-11T16:31:40Z

@bartlomieju
bundle operation requires module_graph::Graph, while run requires deno_core::ModuleSpecifier. This is why I avoided using the word graphas function name.
But indeed watch_func_with_module_resolution is long and kind of redundant, so I'd be happy to find appropriate name...

cli/file_watcher.rs

magurotuna · 2020-11-17T15:58:09Z

I added tests for run and bundle in the following cases:

watcher exits immediately if module resolution fails at the first attempt due to invalid syntax
watcher tries to continue watching files using previously resolved data

magurotuna · 2020-11-18T07:02:29Z

hmmm..... tests are sooooo flaky :(

bartlomieju · 2020-11-18T13:17:55Z

hmmm..... tests are sooooo flaky :(

@magurotuna are you talking about tests for this PR or tests in general?

magurotuna · 2020-11-18T13:26:28Z

@bartlomieju
Seems like both of them.

https://github.com/denoland/deno/runs/1413182650#step:16:3855 -> operation timeout on macOS, which happened at the place unrelated to this PR
https://github.com/denoland/deno/runs/1416308269#step:14:12158 -> rusty_v8 build failed on ubuntu, totally unrelated to this PR
https://github.com/denoland/deno/runs/1416415130#step:16:3862 -> fmt_watch_test failed on macOS. This seems to be related to this PR and "run_watch" test is flaky #8316.

I have no idea why fmt_watch_test failed. I tried to run the tests on my local mac, they were totally fine.

bartlomieju · 2020-11-18T13:27:58Z

@magurotuna thanks for detailed description. I'm aware of the first problem, but haven't found the reason yet. Second failure seems like an intermittent failure. As for third I'm not sure, let's keep a close eye on it.

magurotuna · 2020-11-20T16:18:48Z

Several times of resolving conflict have been a little bit grunt work 😢

bartlomieju · 2020-11-21T13:25:10Z

Several times of resolving conflict have been a little bit grunt work 😢

Sorry for that @magurotuna, I'll try to land your PR this weekend.

Can I ask you to take a look at this comment and check if that's still the case?

magurotuna · 2020-11-21T14:05:19Z

@bartlomieju Sure, I'll check for that.

bartlomieju · 2020-11-21T17:17:40Z

@magurotuna I've tested this PR locally and it's working very well! Code in this PR is a big improvement compared to current implementation and LGTM. Please prepare commit message before landing :)

magurotuna · 2020-11-22T07:39:00Z

@bartlomieju
Thanks for your test!

Please prepare commit message before landing :)

Sorry if I misunderstand it, but does it mean that you want me to write a commit message that will be used in a squashed merge commit?

bartlomieju · 2020-11-22T12:32:03Z

Sorry if I misunderstand it, but does it mean that you want me to write a commit message that will be used in a squashed merge commit?

Yes, exactly

magurotuna · 2020-11-22T18:08:54Z

@bartlomieju
Sure, I've added a brief message to the head of the PR's body. Is this fine?

bartlomieju

LGTM

…enoland#8276) This commit adds support for "--watch" flag for "bundle" and "fmt" subcommands. In addition to this, it refactors "run --watch" command so that module resolution will occur every time the file watcher detects file addition/deletion, which allows the watcher to observe a file that is newly added to the dependency as well.

bartossh and others added 11 commits September 30, 2020 13:08

refactor fmt to watch files

d4f8cdc

test watch fmt

23267d8

Merge branch 'master' into watch_fmt

1e25d07

wip

a2c2239

use Delay instead of Interval

bb1bd6b

refactor

f8aec3e

remove watch_func

21b677e

Merge branch 'master' into watcher-module-resolve

93278bc

fix debounce interval ms

a42da3e

add test

0bcd86b

clippy

74859ee

magurotuna changed the title ~~fix(cli/watcher): detect changes happening on files newly added to dependency~~ fix(cli/watcher): detect changes on files newly added to dependency Nov 7, 2020

magurotuna marked this pull request as ready for review November 7, 2020 07:48

magurotuna added 6 commits November 10, 2020 23:06

Merge branch 'master' into watcher-module-resolve

38dbf79

Merge branch 'watch_fmt' into watcher-module-resolve

ff6c8f2

fmt --watch

7a665f2

fix test

6614428

refactor

f6b3479

clippy

241856d

magurotuna added 6 commits November 11, 2020 23:43

add --watch flag to bundle

ef298c1

bundle works with --watch flag

677737f

rename variable

4e514a3

fix

640079f

add bundle watch test

69aff23

fix tests

d111d9c

clippy

9f11a9b

magurotuna added 2 commits November 17, 2020 22:12

Merge branch 'master' into watcher-module-resolve

a5655ab

try to continue watching if module resolution fails

c9eda36

magurotuna commented Nov 17, 2020

View reviewed changes

cli/file_watcher.rs Outdated Show resolved Hide resolved

magurotuna added 2 commits November 17, 2020 23:39

clippy

019789e

add tests

b8efd08

magurotuna added 3 commits November 18, 2020 14:01

ci

c99c630

why rusty_v8 build failed :/

a35de82

test passes on macOS locally...

998553f

Merge branch 'master' into watcher-module-resolve

a0f1a67

Merge branch 'master' into watcher-module-resolve

a17007d

bartlomieju changed the title ~~fix(cli/watcher): detect changes on files newly added to dependency~~ feat(unstable): Support --watch flag for bundle and fmt subcommands Nov 21, 2020

magurotuna added 2 commits November 22, 2020 15:06

Merge branch 'master' into watcher-module-resolve

05aaf55

remove FIXME comment as fixed

23d62e6

Merge branch 'master' into watcher-module-resolve

0a48ddc

bartlomieju approved these changes Nov 22, 2020

View reviewed changes

bartlomieju merged commit e3f73d3 into denoland:master Nov 22, 2020

magurotuna deleted the watcher-module-resolve branch November 23, 2020 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(unstable): Support --watch flag for bundle and fmt subcommands #8276

feat(unstable): Support --watch flag for bundle and fmt subcommands #8276

magurotuna commented Nov 7, 2020 •

edited

Loading

magurotuna commented Nov 10, 2020

magurotuna commented Nov 11, 2020

bartlomieju commented Nov 11, 2020

magurotuna commented Nov 11, 2020 •

edited

Loading

magurotuna commented Nov 17, 2020

magurotuna commented Nov 18, 2020

bartlomieju commented Nov 18, 2020

magurotuna commented Nov 18, 2020

bartlomieju commented Nov 18, 2020

magurotuna commented Nov 20, 2020

bartlomieju commented Nov 21, 2020

magurotuna commented Nov 21, 2020

bartlomieju commented Nov 21, 2020

magurotuna commented Nov 22, 2020

bartlomieju commented Nov 22, 2020

magurotuna commented Nov 22, 2020

bartlomieju left a comment

feat(unstable): Support --watch flag for bundle and fmt subcommands #8276

feat(unstable): Support --watch flag for bundle and fmt subcommands #8276

Conversation

magurotuna commented Nov 7, 2020 • edited Loading

TODO

magurotuna commented Nov 10, 2020

magurotuna commented Nov 11, 2020

bartlomieju commented Nov 11, 2020

magurotuna commented Nov 11, 2020 • edited Loading

magurotuna commented Nov 17, 2020

magurotuna commented Nov 18, 2020

bartlomieju commented Nov 18, 2020

magurotuna commented Nov 18, 2020

bartlomieju commented Nov 18, 2020

magurotuna commented Nov 20, 2020

bartlomieju commented Nov 21, 2020

magurotuna commented Nov 21, 2020

bartlomieju commented Nov 21, 2020

magurotuna commented Nov 22, 2020

bartlomieju commented Nov 22, 2020

magurotuna commented Nov 22, 2020

bartlomieju left a comment

Choose a reason for hiding this comment

magurotuna commented Nov 7, 2020 •

edited

Loading

magurotuna commented Nov 11, 2020 •

edited

Loading