Provide the watcher on the plugin context #2261
Conversation
| let curWatcher: Watcher; | ||
| export function setWatcher(watcher: Watcher) { | ||
| curWatcher = watcher; | ||
| } |
There was a problem hiding this comment.
This indirection is done so that setting the watcher is a private API and not exposed.
Alternatively the main rollup function could just be wrapped.
There was a problem hiding this comment.
I thought about this but yes, this seems like a reasonable approach.
There was a problem hiding this comment.
On a totally unrelated note, I wonder why the diff in Github looks so messy. IntelliJ is able to create a nice and focused diff for this file. I hope Github can improve this at some point...
There was a problem hiding this comment.
Just discovered that if you select "Diff settings -> Hide Whitespace Changes -> Apply and Reload" this gets cleaned up nicely!
There was a problem hiding this comment.
Looks nice! I hope in the future we could also be able to provide more feedback APIs to enable e.g. more fine-grained progress visualization plugins for the actual bundling process. But I would hope for an external developer to step forward first to ask for this to be able to involve more people here.
test/watch/index.js
Outdated
| 'END', | ||
| () => { | ||
| assert.equal(run('../_tmp/output/bundle.js'), 43); | ||
| watcher.close(); |
There was a problem hiding this comment.
I would also like to check the events again here to see our hook still works.
|
|
||
| export * from './rollup/index'; | ||
| export * from './watch/index'; | ||
|
|
There was a problem hiding this comment.
Interesting. We were doing wild things here. So as long as nobody imported the classes in watch/index.js I guess we should be fine here. Nice catch!
src/watch/index.ts
Outdated
| return new Watcher(configs); | ||
| const watcher = new Watcher(configs); | ||
|
|
||
| return watcher; |
There was a problem hiding this comment.
Any reason for splitting this up?
| let curWatcher: Watcher; | ||
| export function setWatcher(watcher: Watcher) { | ||
| curWatcher = watcher; | ||
| } |
There was a problem hiding this comment.
I thought about this but yes, this seems like a reasonable approach.
| @@ -1,12 +1,12 @@ | |||
| // Return the first non-null or -undefined result from an array of | |||
| // maybe-sync, maybe-promise-returning functions | |||
| export default function first<T>( | |||
| export default function first<T, Context>( | |||
There was a problem hiding this comment.
Not 100% happy about this as this new context handling is not too obvious from the function name. What about making the context and explicit parameter of this function and not add the context if we do not pass the parameter? In that case, I would be ok with keeping the function name as it is.
That would make the comment above true again. Otherwise, this function is now very specific to certain usages and may have unintended side-effects.
There was a problem hiding this comment.
Also, why did you perform this change? It touches quite a bit of existing functionality.
There was a problem hiding this comment.
This function is only used for calling plugin hooks. And all plugin hooks now need context to be passed through it. It seems like a natural extension to me that the caller should set the context for the individual functions, as if the caller calls all functions as a single function.
There was a problem hiding this comment.
Maybe. But it also sets the context if the caller does not want to set the context. But I guess this doesn't matter. But then, firstSync should be changed as well for symmetry.
| let curWatcher: Watcher; | ||
| export function setWatcher(watcher: Watcher) { | ||
| curWatcher = watcher; | ||
| } |
There was a problem hiding this comment.
On a totally unrelated note, I wonder why the diff in Github looks so messy. IntelliJ is able to create a nice and focused diff for this file. I hope Github can improve this at some point...
lukastaegert
force-pushed
the
watcher-plugin-context
branch
from
0b543d7
to
4b60322
Compare
|
It appears this PR has broken the memory leak test. Will try to find out if this can be fixed easily so that master can be released. |
|
Found and pushed a fix for the leak: 2361e3d#diff-c5fd8e65630bd641bf07130346ee8778R173 (Sorry about the diff, it's mostly changed indentation) Not sure what the actual cause way. I reverted the rather cosmetic change of replacing the try-catch with running the initial code inside a promise. |
This Pull Request updates dependency [rollup](https://github.com/rollup/rollup) from `v0.60.7` to `v0.61.1` <details> <summary>Release Notes</summary> ### [`v0.61.1`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0611) [Compare Source](rollup/rollup@v0.61.0...697f36d) *2018-06-21* * Do not try to deconflict "undefined" ([#​2291](`rollup/rollup#2291)) * Properly track values for loop interator declarations and reassigned namespaces, add smoke test ([#​2292](`rollup/rollup#2292)) --- ### [`v0.61.0`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0610) [Compare Source](rollup/rollup@v0.60.7...v0.61.0) *2018-06-20* * Declare file dependencies via transform plugin hooks ([#​2259](`rollup/rollup#2259)) * Handle undefined values when evaluating conditionals ([#​2264](`rollup/rollup#2264)) * Handle known undefined properties when evaluating conditionals ([#​2265](`rollup/rollup#2265)) * Access watch events via the plugin context ([#​2261](`rollup/rollup#2261)) * Add option to suppress `__esModule` flag in output ([#​2287](`rollup/rollup#2287)) * Fix issue when re-declaring variables, track reassignments in more cases ([#​2279](`rollup/rollup#2279)) * Add VSCode debug settings ([#​2276](`rollup/rollup#2276)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
This allows for plugins that can track the watcher lifecycle like notifiers. Resolves #2117.