Pass watchedNodes to Watcher/WatcherAdapter #403
Conversation
|
@oligriffiths Should this be apart of the 3.0 release or should it come after? |
|
@thoov I’d prefer if it were but I’m not going to be able to work much more on it until Monday |
|
Just wondering, if it's a have sence - use current git revision for trigger rebuild if it changes (for upstream changes) or use requests from IDE like VSCode language server to get exact changed files (without watching overhead) |
|
@lifesrt the fit revision doesn’t know about changed files, only committed ones. Not sure how this would help? |
|
@oligriffiths This looks good. Just to confirm that this is so we dont have to do this? (https://github.com/ember-cli/ember-cli/pull/8614/files#diff-9ff56008b40607a31bc04df7f41906cbR45) |
|
@thoov correct. Also, the things to watch (nodes) are a direct dependency of this class. To me it makes more sense for them to be a mandatory argument rather than reaching in to the builder to get them |
| @@ -199,7 +202,7 @@ describe('Watcher', function() { | |||
|
|
|||
| it('does nothing if already quitting', function() { | |||
| const adapterQuit = sinon.spy(adapter, 'quit'); | |||
| const watcher = new Watcher(builder, { watcherAdapter: adapter }); | |||
| const watcher = new Watcher(builder, [watchedNodeBasic], { watcherAdapter: adapter }); | |||
| watcher._quittingPromise = true; | |||
There was a problem hiding this comment.
using private API in tests is an anti pattern, let's try and use public API's
|
@lifeart I think asking git, for what is different, isn't a bad idea. We should explore that sometime. That being said, without polling git, I don't think git can notify us of a change. |
* ensure new watchedNodes are passed to the watchAdapter * remove some mocks/stubs (we should try and remove the rest at some point) as they break encapsulation * Separate serve from it’s implementation, Server can now be tested independently of mocking `process` (fixed flakey test) * fix some mocks/spys now that internal implementation have changed (TODO: we need to remove many of these mocks/spys) * fix race condition in dummy watcher * fix lib/cli’s server actionPromise, which had code in it to support a broken mock
stefanpenner
force-pushed
the
watcher-constructor
branch
from
5475bf3
to
e8aa89c
Compare
oligriffiths
changed the title
| @@ -10,28 +10,30 @@ const logger = require('heimdalljs-logger')('broccoli:watcher'); | |||
| // principle. | |||
|
|
|||
| module.exports = class Watcher extends EventEmitter { | |||
| constructor(builder, options) { | |||
| constructor(builder, watchedNodes = [], options = {}) { | |||
There was a problem hiding this comment.
If the watched nodes are required, why do we default them?
| if (this.options.debounce == null) { | ||
| this.options.debounce = 100; | ||
| } | ||
| this.builder = builder; | ||
| this.watcherAdapter = | ||
| this.options.watcherAdapter || new WatcherAdapter(this.options.saneOptions); | ||
| this.options.watcherAdapter || | ||
| new WatcherAdapter(watchedNodes || [], this.options.saneOptions); |
There was a problem hiding this comment.
Why do we default watchedNodes twice?
| @@ -18,20 +19,29 @@ function bindFileEvent(adapter, watcher, node, event) { | |||
| } | |||
|
|
|||
| module.exports = class WatcherAdapter extends EventEmitter { | |||
| constructor(options) { | |||
| constructor(watchedNodes = [], options) { | |||
There was a problem hiding this comment.
IMHO, we should just require an arg here. This defaulting is a bit odd (e.g. it only supports new WatcherAdapter(undefined, { /* options here */ })).
| @@ -591,18 +595,22 @@ describe('cli', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| // TODO: remove these mocks and spys | |||
There was a problem hiding this comment.
Can we file an issue to keep track of this?
|
@rwjblue valid points, will fix |
The intention of this PR is to make the
watchedNodesan argument to the constructor of the watcher, as they're a required dependency and watcher can't work without them.Additionally, NOT reaching into the builder to get them will help with the ember CLI integration of the watcher.