hapijs / glue

Server composer for hapi.js
Other
245 stars 62 forks source link

feat(extensions): add extension support to manifest #125

Closed Crazyglue closed 5 years ago

Crazyglue commented 5 years ago

Thought it would be helpful to also register extensions using a manifest!

csrl commented 5 years ago

I've generally considered the manifest to be something that can be serialized. ie. loaded from a text file.

I'm not necessarily opposed to this change, but I'm wondering what it benefits?

Crazyglue commented 5 years ago

Ive found myself needing to register top-level extensions that dont really have a place in a specific plugin. For example, I may have an extension that needs to fetch user data (or use cached user data) and add that into the context of most view-requests (so that down the line, handlebars can inject that into the page's template, in the header).

I dont really see that living as a plugin, since its just a single request hook and we dont have any other business logic related to user data anywhere in the app.

We also have extensions which parse the user's locale and provides that in the context.

Its little tiny extensions like that (which are needed at an app-level) that I see benefiting from this change. To the point of loading from a text file, this wouldnt actually effect existing functionality, but yes the end user would have to const manifest = require('manifest.json') then extend it to map their extensions. I dont see that as a huge deal since you can abstract the extension registration part away with something like manifest.register.extensions = require('path/to/extensions/index')

csrl commented 5 years ago

What's the difference between modifying the manifest prior to glue.compose, or using the 'server' in the preRegister handler, or just using the server returned from glue.compose and calling server.ext(extensions);

I guess I'm failing to see the point of extending the manifest programmatically vs just calling server.ext directly. What does it solve?

hueniverse commented 5 years ago

This does not belong here. The manifest must be pure JSON.

csrl commented 5 years ago

With all versions of glue, the manifest in fact can be a non JSON compatible object already, as seen in the test case composes a server with server.cache.provider as a function.

Anyway, for this particular request, I don't see the value as implemented, so 'closed' is fine.

Thanks @Crazyglue for the PR though! I'm still interested in how you are using glue such that you want to programmatically generate your manifest, vs just directly composing the server yourself. ie why use glue at all then? Its purpose is configuration based composition.

I'm still open to supporting server.ext or any other server capability, such that it improves the configuration based (vs programmatic) composition of the server.

hueniverse commented 5 years ago

Still against the spirit of what this module is for.

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.