-
-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support loading of plugins in a smart order based on dependency #30
Conversation
Hi, Could you fix the error for Node.js 6? Meanwhile, I'll check deeper the code, for now, I'm thinking that a hidden limit is that it will be not possible load twice a plugin with the same name but I need to think if it is a real use case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! Instead of extending the “basic” app, can you create a new one?
if (plugin.prefixOverride !== void 0) { | ||
pluginOptions.prefix = plugin.prefixOverride | ||
} | ||
|
||
if (plugin.autoload !== false) { | ||
fastify.register(plugin.default || plugin, pluginOptions) | ||
allPlugins[pluginName] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should throw if two plugins have the same name.z
Thanks @mcollina, done those changes. |
@Eomm I tried to think about the hidden limit you mentioned. Loading a plugin twice in the same load chain is a result of cyclic dependency. I couldn't think of a case where one would load a plugin twice in the same load chain and still not cause cyclic dependency. It's however fine to load a plugin from two separate load sequence. i.e. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some decorators/checks in the dependency test plugins so that it won't load unless we the order is correct?
Also, CI is failing. |
Yep, good idea. All done now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add 2 hints
@Eomm Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will release it by the week end as semver-minor 👍
Fixes #29
dependencies
array).