Skip to content
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

Add fastify-loader to Ecosystem.md #1154

Merged
merged 1 commit into from
Sep 11, 2018
Merged

Conversation

TheNoim
Copy link
Contributor

@TheNoim TheNoim commented Sep 9, 2018

Checklist

  • documentation is changed or added
  • commit message and code follows Code of conduct

Link: fastify-loader

@cemremengu
Copy link
Contributor

How is this different than using https://github.com/fastify/fastify-autoload ?

@TheNoim
Copy link
Contributor Author

TheNoim commented Sep 9, 2018

@cemremengu You don't need to write

module.exports = function (fastify, opts, next) {

The fastify instance gets injected.
For me module.exports does not look clean.

Edit:
And I use globs, fastify-autoload only loads a hole directory. You have the possibility to use ignorePattern, but I think the glob approach is better.

@mcollina
Copy link
Member

mcollina commented Sep 9, 2018

I'm kind of -1 into publicizing modules that monkeypatch the Node.js loader. I would discourage people to use this: https://github.com/TheNoim/fastify-loader/blob/master/inject.js.

Also, exporting a function and passing in the fastify instance ensures that multiple copies of the same fastify app could get started (in tests, for examples).

@TheNoim
Copy link
Contributor Author

TheNoim commented Sep 9, 2018

@mcollina

I would discourage people to use this

You can. But everyone propably should decide on their own.

Also, exporting a function and passing in the fastify instance ensures that multiple copies of the same fastify app could get started (in tests, for examples).

Yes this is right. But in most cases, this isn't needed. You can see this more like an: I want to make a quick webserver for a small project with as less stumbling stones as possible.

@mcollina
Copy link
Member

mcollina commented Sep 9, 2018

Yes this is right. But in most cases, this isn't needed. You can see this more like an: I want to make a quick webserver for a small project with as less stumbling stones as possible.

This is needed in most cases, as it will prevent writing effective unit tests.

It is possible to achieve something very similar with the use of the vm core module and a Proxy object for the global context. I would recommend pursuing that route instead (and it would indeed be very cool). It might require a bit more code, but it would support writing tests effectively.

@TheNoim
Copy link
Contributor Author

TheNoim commented Sep 9, 2018

This is needed in most cases, as it will prevent writing effective unit tests.

Pff.. Untis test, my most hated thing. :P

Edit:

You can pass multiple fastify instances. You can change the instance name.

It is possible to achieve something very similar with the use of the vm core module and a Proxy object for the global context. I would recommend pursuing that route instead (and it would indeed be very cool). It might require a bit more code, but it would support writing tests effectively.

This was the first thing I tried. But if you provide a context, it gets sandboxed. This prevents to apply changes made to the fastify instance. The only alternative would be eval. But eval has access to the lcoal scope. Another alternative is vm2. The problem with vm2 is, you need to mock __dirname and __filename. Oh you don't need to.

@TheNoim
Copy link
Contributor Author

TheNoim commented Sep 9, 2018

@mcollina Updated the module. Replace this "hacky" method with vm2.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

Good work! It would be fantastic to add a unit test to load 2 fastify apps in the same process ;).

@mcollina mcollina merged commit 9e11a69 into fastify:master Sep 11, 2018
@delvedor delvedor added documentation Improvements or additions to documentation plugin Identify a pr to the doc that adds a plugin. labels Sep 24, 2018
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation plugin Identify a pr to the doc that adds a plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants