#315 - Add watch-compile event #319

dwelch2344 · 2018-02-05T17:48:16Z

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/wpwatch
that 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-compile definition in index.js and a serverless.spawn of said event in lib/wpwatch.js

How can we verify it:

Install locally to your npm
Clone this project: https://github.com/dwelch2344/serverless-webpack-watch-compile-demo
Adjust the project's package.json to use your local version
run npm run debug and observe the plugin logs 15 times:

→ npm run debug

> [email protected] debug /tmp/serverless-webpack-watch-compile
> node --inspect ./node_modules/.bin/serverless offline -P 5000 -s dev

Debugger listening on ws://127.0.0.1:9229/8ee583a5-086a-407b-829d-c1a971a4d39f
For help see https://nodejs.org/en/docs/inspector
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Bundling with Webpack...
Time: 519ms
         Asset     Size  Chunks             Chunk Names
    handler.js  5.52 kB       0  [emitted]  handler
handler.js.map  5.38 kB       0  [emitted]  handler
   [0] ./handler.js 2.35 kB {0} [built]
   [1] external "babel-runtime/core-js/promise" 42 bytes {0} [not cacheable]
   [2] external "babel-runtime/helpers/objectWithoutProperties" 42 bytes {0} [not cacheable]
   [3] external "babel-runtime/regenerator" 42 bytes {0} [not cacheable]
   [4] external "babel-runtime/core-js/json/stringify" 42 bytes {0} [not cacheable]
   [5] external "babel-runtime/helpers/asyncToGenerator" 42 bytes {0} [not cacheable]
   [6] external "source-map-support/register" 42 bytes {0} [not cacheable]
Serverless: Watching for changes...
Serverless: Starting Offline: dev/us-east-1.

Serverless: Routes for hello:
Serverless: GET /hello

Serverless: Offline listening on http://localhost:5000

Now make a change in handler.js, and observe the plugin logs again

Time: 88ms
         Asset     Size  Chunks             Chunk Names
    handler.js  5.52 kB       0  [emitted]  handler
handler.js.map  5.38 kB       0  [emitted]  handler
   [0] ./handler.js 2.36 kB {0} [built]
   [1] external "babel-runtime/core-js/promise" 42 bytes {0} [not cacheable]
   [2] external "babel-runtime/helpers/objectWithoutProperties" 42 bytes {0} [not cacheable]
   [3] external "babel-runtime/regenerator" 42 bytes {0} [not cacheable]
   [4] external "babel-runtime/core-js/json/stringify" 42 bytes {0} [not cacheable]
   [5] external "babel-runtime/helpers/asyncToGenerator" 42 bytes {0} [not cacheable]
   [6] external "source-map-support/register" 42 bytes {0} [not cacheable]
Serverless: Watching for changes...
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected
Serverless: Running after a compiled change detected

Verify your change is there and things worked as expected

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification config / commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

…vent control while doing local development

HyperBrain

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).

HyperBrain · 2018-02-05T18:20:25Z

index.js

@@ -72,6 +72,12 @@ class ServerlessWebpack {
              'compile',
            ],
          },
+          "watch-compile": {


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.

HyperBrain · 2018-02-05T18:22:00Z

lib/wpwatch.js

@@ -51,6 +51,8 @@ module.exports = {
        if (firstRun) {
          firstRun = false;
          callback();
+        }else{
+          this.serverless.pluginManager.spawn('webpack:watch-compile')


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.

HyperBrain · 2018-02-05T18:25:16Z

If you need any help, please let me know.

dwelch2344 · 2018-02-05T18:31:11Z

Great feedback, thank you. Only one question:

And you should spawn also for the other watch commands (with invoke local and run).
Not sure I follow exactly. What does that entail / look like?

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!

HyperBrain · 2018-02-05T18:35:08Z

The other kinds of watches are (quite similar code) in run.js

HyperBrain · 2018-02-05T18:36:54Z

... 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.

…tions

dwelch2344 · 2018-02-06T14:39:09Z

@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 run.test.js as well as wpwatch.test.js and coverage has actually gone up :)

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?

HyperBrain · 2018-02-06T14:48:25Z

@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):
With awaiting the spawn I meant that now the hooks are triggered by the watch implementation - which is perfectly done how it is. But the spawn() itself is not awaited within the watch callback, what means that it can happen that the hook is triggered, dependent plugins do something, but as the callback already returned, an additional hook invocation can happen while the previous one is not yet finished.

That might actually never happen ;-) - or can be fixed if it happens.

HyperBrain · 2018-02-06T15:14:54Z

@dwelch2344 Just saw that the new hookable event should be added to the README (section "for developers"). Can you do that?

HyperBrain · 2018-02-06T17:57:04Z

I just restarted the build (there is something not ok with the validate unit tests - but that's out of scope here). Now it succeeded.

dwelch2344 · 2018-02-06T19:15:42Z

@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?

HyperBrain · 2018-02-06T19:43:52Z

do I simply need to "return" the spawn invocation?

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.

dwelch2344 · 2018-02-06T21:09:38Z

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

HyperBrain · 2018-02-07T12:42:05Z

@dwelch2344 I commited a fix for the race condition and tested it locally with serverless offline. Now the event code is awaited correctly and watching resumes after the events have been processed.
Additionally I fixed a generic issue with unnecessary recompiles by checking the webpack compile hash as proposed on their site.
Can you pull the branch and do some tests too? We have to make sure that it now works expected before it can be merged.

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 😄

dwelch2344 · 2018-02-07T17:36:10Z

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 :)

dwelch2344 · 2018-02-08T04:12:20Z

Just confirmed! this looks great! Can we go with it?

HyperBrain · 2018-02-08T09:35:08Z

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

dwelch2344 · 2018-02-08T15:44:12Z

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 ?

HyperBrain · 2018-02-09T11:35:11Z

I'll postpone the event trigger for local invoke and run to a second PR, because that implementation has to be cleaned up anyway.
You'll hear from me via DM this weekend ;-)

…e-hook serverless-heaven#315 - Add watch-compile event

serverless-heaven#315 - Add watch-compile event for more fine grain e…

97666b3

…vent control while doing local development

dwelch2344 mentioned this pull request Feb 5, 2018

Question: hooks *after* compile – but *before* run #315

Closed

HyperBrain requested changes Feb 5, 2018

View reviewed changes

dwelch2344 added 2 commits February 6, 2018 07:18

319 - Added test coverage + renamed hooks, per @HyperBrain recommenda…

b14bc9c

…tions

lint fix :)

599cb4b

Documentation for compile:watch:compile hook

7474d3c

Fixed race condition when triggering watch event

f4f0759

Frank Schmid added 2 commits February 7, 2018 13:53

Fixed unit test and comment

b2882c8

Test new behavior in wpwatch unit tests

3524849

HyperBrain added this to the 4.3.0 milestone Feb 7, 2018

HyperBrain approved these changes Feb 8, 2018

View reviewed changes

HyperBrain merged commit 66cb262 into serverless-heaven:master Feb 9, 2018

dwelch2344 deleted the watch-compile-hook branch February 9, 2018 18:33

deftomat mentioned this pull request Nov 23, 2018

Unexpected watcher stops #465

Closed

This was referenced Mar 2, 2019

Did 4.0.0+ become incompatible with awesome-typescript-loader & hot-reload? #449

Closed

Converted the watcher.stop() to use built in webpack hooks instead #489

Merged

jamesmbourne pushed a commit to jamesmbourne/serverless-webpack that referenced this pull request Oct 15, 2019

Merge pull request serverless-heaven#319 from dwelch2344/watch-compil…

71b998f

…e-hook serverless-heaven#315 - Add watch-compile event

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#315 - Add watch-compile event #319

#315 - Add watch-compile event #319

dwelch2344 commented Feb 5, 2018

HyperBrain left a comment

HyperBrain Feb 5, 2018 •

edited

Loading

HyperBrain Feb 5, 2018 •

edited

Loading

HyperBrain commented Feb 5, 2018

dwelch2344 commented Feb 5, 2018

HyperBrain commented Feb 5, 2018

HyperBrain commented Feb 5, 2018 •

edited

Loading

dwelch2344 commented Feb 6, 2018

HyperBrain commented Feb 6, 2018

HyperBrain commented Feb 6, 2018

HyperBrain commented Feb 6, 2018

dwelch2344 commented Feb 6, 2018

HyperBrain commented Feb 6, 2018 •

edited

Loading

dwelch2344 commented Feb 6, 2018

HyperBrain commented Feb 7, 2018 •

edited

Loading

dwelch2344 commented Feb 7, 2018

dwelch2344 commented Feb 8, 2018

HyperBrain commented Feb 8, 2018

dwelch2344 commented Feb 8, 2018

HyperBrain commented Feb 9, 2018

#315 - Add watch-compile event #319

#315 - Add watch-compile event #319

Conversation

dwelch2344 commented Feb 5, 2018

What did you implement:

How did you implement it:

How can we verify it:

Todos:

HyperBrain left a comment

Choose a reason for hiding this comment

HyperBrain Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

HyperBrain Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

HyperBrain commented Feb 5, 2018

dwelch2344 commented Feb 5, 2018

HyperBrain commented Feb 5, 2018

HyperBrain commented Feb 5, 2018 • edited Loading

dwelch2344 commented Feb 6, 2018

HyperBrain commented Feb 6, 2018

HyperBrain commented Feb 6, 2018

HyperBrain commented Feb 6, 2018

dwelch2344 commented Feb 6, 2018

HyperBrain commented Feb 6, 2018 • edited Loading

dwelch2344 commented Feb 6, 2018

HyperBrain commented Feb 7, 2018 • edited Loading

dwelch2344 commented Feb 7, 2018

dwelch2344 commented Feb 8, 2018

HyperBrain commented Feb 8, 2018

dwelch2344 commented Feb 8, 2018

HyperBrain commented Feb 9, 2018

HyperBrain Feb 5, 2018 •

edited

Loading

HyperBrain Feb 5, 2018 •

edited

Loading

HyperBrain commented Feb 5, 2018 •

edited

Loading

HyperBrain commented Feb 6, 2018 •

edited

Loading

HyperBrain commented Feb 7, 2018 •

edited

Loading