feat: add waitForBuild option #102
Conversation
smashercosmo
force-pushed
the
wait-for-bundle-ready
branch
from
9687a2a to
04722e1
Compare
smashercosmo
changed the title
smashercosmo
force-pushed
the
wait-for-bundle-ready
branch
from
04722e1 to
9bbce13
Compare
matheus1lva
left a comment
matheus1lva
left a comment
There was a problem hiding this comment.
For what i could see from previous comments on old issues, this can decrease the number of weird errors, specially from websocket side.
Also, don't forget to update the typescript definition file. Almost the same as the joi validation.
smashercosmo
force-pushed
the
wait-for-bundle-ready
branch
from
9bbce13 to
a071e80
Compare
|
@playma256 thx for review. definitions are updated now. |
|
Great work and thank you for the PR. Please give me some time to consider the verbiage of the option name and documentation. In the mean time, it looks like Node 10 tests are failing, and those need to be resolved. |
smashercosmo
force-pushed
the
wait-for-bundle-ready
branch
from
a071e80 to
e7d6238
Compare
|
Alright. Tests are passing now. |
|
I'm not quite sure, how to write test to cover bundle rebuilding case. |
You'd have to overwrite a file that's in the bundle to force a rebuild, and then wait for that rebuild to finish. I think we have some tests that do something similar, but if it works for the first build. it's probably going to work for the second, third, etc @sibelius please give this PR a look when you get the chance |
shellscape
left a comment
shellscape
left a comment
There was a problem hiding this comment.
A few semantic changes I'd like to make.
smashercosmo
force-pushed
the
wait-for-bundle-ready
branch
from
8c7e965 to
570f855
Compare
smashercosmo
force-pushed
the
wait-for-bundle-ready
branch
from
570f855 to
a38305e
Compare
smashercosmo
changed the title
|
Ready for final review |
lib/index.js
Outdated
| options.static = []; | ||
| } | ||
|
|
||
| if (options.waitForBuild) { |
There was a problem hiding this comment.
why do we need a done handler here as well as on line 170? is it to catch the first build and then any subsequent builds?
lib/server.js
Outdated
|
|
||
| if (waitForBuild) { | ||
| app.use(async (ctx, next) => { | ||
| if (!this.compiled) { |
There was a problem hiding this comment.
since the next line is just awaiting a promise, we don't need a logical check here. we can always await the promise. once the promise is resolved, this will pass-through. I'll push this change to your branch.
|
I'm working on a few changes to the PR now. One big problem that strikes me is probably going to be |
| if (this.options.waitForBuild) { | ||
| // track the first build of the bundle | ||
| this.state.compiling = new Promise((resolve) => { | ||
| this.once('done', () => resolve()); |
There was a problem hiding this comment.
Can be shortened to this.once('done', resolve);
There was a problem hiding this comment.
it could :) in certain situations I like to be a bit more verbose.
| // track subsequent builds from watching | ||
| this.on('invalid', () => { | ||
| this.state.compiling = new Promise((resolve) => { | ||
| this.once('done', () => resolve()); |
3 participants
This PR contains:
Breaking Changes?
Please Describe Your Changes
This PR adds 'waitForBundleReady' option, which prevents all server middlewares to run until bundle is fully compiled. Same functionality is supported by webpack-dev-middleware via serverSideRender option