Added an autostart flag to defer loading #38
Conversation
|
Excellent. |
| Plugin.loadPlugin.call(this, main, (err) => { | ||
| debug('root plugin ready') | ||
| if (err) { | ||
| this._error = err |
There was a problem hiding this comment.
Unless this._doStart is called with an error argument this is unreachable. I found this while working on a patch to increase test coverage.
There was a problem hiding this comment.
Not sure what happened, my code selection got moved up a couple lines. should have selected the lines within if (err) {...}.
There was a problem hiding this comment.
This is not correct. Here we receive the errors of all the loaded plugins.
There was a problem hiding this comment.
I tried to get an error to reach this block, I was unsuccessful so I analyzed the code. I'm confident that it's unreachable but that's not important to this review.
| if (this.booted) { | ||
| throw new Error('root plugin has already booted') | ||
| } | ||
| Boot.prototype.start = function () { |
There was a problem hiding this comment.
Should this be added to wrap?
Also if this.started is already true do we want to run this._doStart again?
There was a problem hiding this comment.
I don’t think we should add it to wrap? Why would you want to do it?
An avvio instance can be booted only once. Otherwise too many edge cases happens (this is discussed elsewhere).
There was a problem hiding this comment.
Nevermind I looked at the docs again it explicitly states which functions are exposed. It needs to be updated to list close and onClose but that's not related to this review. I'll submit a doc update for that later.
This should solve fastify/fastify#491
cc @jsumners