#315 - Add watch-compile event #319
Conversation
…vent control while doing local development
There was a problem hiding this comment.
Hi @dwelch2344 , good addition!
Please check my comments. Additionally, there should be unit tests to keep coverage high (check in the watch unit tests that the spawn is called). And you should spawn also for the other watch commands (with invoke local and run).
index.js
Outdated
| @@ -72,6 +72,12 @@ class ServerlessWebpack { | |||
| 'compile', | |||
| ], | |||
| }, | |||
| "watch-compile": { | |||
There was a problem hiding this comment.
You should move that as subcommand to the compile command (as entrypoint):
compile:
...
commands: {
watch: {
type: 'entrypoint',
lifecycleEvents: [
'compile'
]
}
}Then it is more universal and better located semantically.
Additionally add a hook to define the new hookable event in this.hooks like this:
this.hooks = [
...
'webpack:compile:watch:compile': () => BbPromise.resolve();
};so that it is owned by the plugin and can be addressed with
after:webpack:compile:watch:compile and the symmetric before.
The new event should be mentioned in the README too.
lib/wpwatch.js
Outdated
| @@ -51,6 +51,8 @@ module.exports = { | |||
| if (firstRun) { | |||
| firstRun = false; | |||
| callback(); | |||
| }else{ | |||
| this.serverless.pluginManager.spawn('webpack:watch-compile') | |||
There was a problem hiding this comment.
This will be webpack:compile:watch then.
We might want to await the promise (spawn) here instead of immediately returning from the callback.
Otherwise we'd create a race condition if the next recompile is done while the event is still triggered.
|
If you need any help, please let me know. |
|
Great feedback, thank you. Only one question:
Again, fairly new to plugin development (and Serverless as a whole, actually) – but happy to do the legwork + reading if you can point me in the right direction / give me a few examples. Thanks! |
|
The other kinds of watches are (quite similar code) in |
|
... and no problem that you're a newcomer to plugin development 😃 . I'm happy to guide you - and if you have generic questions about the Serverless plugin system, it's also no problem for me to help. |
|
@HyperBrain Okay, I think we're good. I've moved to the subcommand (nifty – definitely like those) and verified things still work as expected. Also adjusted tests in both Regarding spawning on the other watch commands: I think this particular hook is right where it needs to be, as it really is only on subsequent compiles via watch that we want to hook in the event (and thus the tests ensuring we don't spawn with 1 invocation but do with 2+ invocations). Is there a gap I'm missing / flaw in my thinking there? |
|
@dwelch2344 Cool, I'll have a look later 😄 The current implementation you did is fine and should serve its purpose. The only thing I mentioned before was this (maybe minor and optional thing): That might actually never happen ;-) - or can be fixed if it happens. |
|
@dwelch2344 Just saw that the new hookable event should be added to the README (section "for developers"). Can you do that? |
|
I just restarted the build (there is something not ok with the |
|
@HyperBrain Ah thank you for the clarification! Yeah, that definitely makes sense and we should do that to keep things running in the right / clean order. Would hate to leave a landmine for someone (especially since we'll be using this soon 😄 ) So, to await for it, do I simply need to "return" the spawn invocation? Assuming it's a promise, and since it's the last call in this instance, I'd think that would do it (based on the other spawn calls above). Is that accurate? If not, mind pointing out an example? |
I think that will not work, because the function we're calling the spawn in is webpack's watch callback - and I doubt that webpack will await it. We might need some more sophisticated stuff here, that will only return from the callback when the spawn returns. Something that is similar to a lock/wait mechanism in other languages, that blocks the callback function until it is released. I'll try to investigate a proper solution tomorrow that handles a wait for a promise in a synchronous function. In theory the callback function itself must be wrapped somehow .... but let's see. Queuing the spawn calls would probably not be an option, as the hookers might depend on the compile output's immutability An option could be using a generator for or in the watch callback function. According to http://node.green/ they are supported down to Node 4.x (with some restrictions on functionality), so that the Serverless requirements are met. I'll do some experiments tomorrow, to see if that can effectively "stall" our callback and wait for the promise to return. |
|
Ok. Quick question then: how do releases on this project work? Aka how long would it take to get this out into a release? If they're small / fast cycles for this kind of thing, could we cut a release and open an enhancement to do the proper solution (which I'm happy to continue work on)? We've got a pretty nifty setup and this is the last piece we need to continue rolling our product out, so while we're monkey-patching it now to dev it'd be nice to sew it up asap. Which I realize is trying to make our problem yours, but figured worth asking 😄 Thanks |
|
@dwelch2344 I commited a fix for the race condition and tested it locally with I have to adapt the unit tests now and change the run watcher in the same way. Afterwards I'll merge if your tests were successful. Regarding the release schedule: There are no fixed release cycles and I generally release, as soon as there are important things available and ready for the next minor or patch release. So we can get this out quite quickly 😄 |
|
This is awesome! Thanks for being so responsive 👍 Will check out and validate ASAP, EOD at the latest. On another note: would love to help contribute and learn from your plugin experience. Could we connect via another channel? Gtalk / twitter / slack / gitter? I'll come to you :) |
|
Just confirmed! this looks great! Can we go with it? |
|
Yes we can 😄 . I'll just check the local invoke watch and the run watch, then we're good to go. ... and of course we can connect - I assume you're in PST, so it's 9 hours difference but still enough overlap :-D |
|
Actually I'm in MT, so 10 probably ;) But I keep odd hours often so happy to work around you. DM me your preferred contact method on twitter at twitter.com/david_welch ? |
|
I'll postpone the event trigger for local invoke and run to a second PR, because that implementation has to be cleaned up anyway. |
…e-hook serverless-heaven#315 - Add watch-compile event
Quick shoutout: I'm new to plugin development and not sure the best way to test / lint / etc, but happy to make changes and do the legwork as needed.
What did you implement:
We've got a plugin in development that needs access to the post-compiled code for our handler, but in local development but there's no way to detect compiled changes if using watch. We've toyed with adding a new event on
lib/wpwatchthat plugins could hook on as needed.
See an example project at https://github.com/dwelch2344/serverless-webpack-watch-compile-demo and the issue #315
filed at #315
How did you implement it:
Added a
watch-compiledefinition inindex.jsand aserverless.spawnof said event inlib/wpwatch.jsHow can we verify it:
package.jsonto use your local versionnpm run debugand observe the plugin logs 15 times:handler.js, and observe the plugin logs againTodos:
Is this ready for review?: YES
Is it a breaking change?: NO