pnxtech / hydra

A light-weight library for building distributed applications such as microservices
https://www.hydramicroservice.com
MIT License
645 stars 54 forks source link

provide default for this.opts in plugins #85

Closed ecwyne closed 7 years ago

ecwyne commented 7 years ago

From what I can tell plugins do not necessarily need options to be passed in. Currently if no hydra.plugins[plugin-name] is provided in the configuration then the plugin initialization throws the error Unhandled rejection TypeError: Cannot read property [plugin-name] of undefined

This PR safely accesses hydra.plugins[plugin-name] and if it doesn't exist then defaults to an empty object.

My preference would be to use pathOr from Ramda.js or get from lodash, but didn't want to add new dependencies without permission.

cjus commented 7 years ago

@emadum thoughts on this? @ecwyne I'd rather not add additional dependencies - in fact I'd like to remove any we currently use can can possibly do without.

emadum commented 7 years ago

I'll defer to @cjus on this but I'm a big fan of lodash and wouldn't mind having it as a dependency. I end up using it in most of my microservices anyway.

:+1: to the PR itself.

cjus commented 7 years ago

The PR looks good to me. Regarding lodash I'd rather see ES6 code fro whatever you would otherwise pull from lodash.

emadum commented 7 years ago

Agreed when possible, but ES6 doesn't have the vast majority of what I use lodash for.

Sent from my iPhone

On Apr 17, 2017, at 5:01 PM, Carlos Justiniano notifications@github.com<mailto:notifications@github.com> wrote:

The PR looks good to me. Regarding lodash I'd rather see ES6 code fro whatever you would otherwise pull from lodash.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fflywheelsports%2Fhydra%2Fpull%2F85%23issuecomment-294592025&data=01%7C01%7Ceric%40flywheelsports.com%7Ce14efab7ec534810f83d08d485d4f2c6%7Ce110a28e5f4f4e558488575a2ed46dbc%7C0&sdata=wUbddqWyKcNrVvGJxV%2Fhij9wvdO%2BymfPhumMEVRL07c%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAASVedune7RnV1VcxHui2PqjfqnYeVmZks5rw9M1gaJpZM4M_bgA&data=01%7C01%7Ceric%40flywheelsports.com%7Ce14efab7ec534810f83d08d485d4f2c6%7Ce110a28e5f4f4e558488575a2ed46dbc%7C0&sdata=Yxg28ik203Fpcw9vgDxn7hvb%2BkWCU88wFpgVjUd7jDg%3D&reserved=0.

This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any errors or omissions in the contents of this message, which arise as a result of e-mail transmission. If verification is required please request a hard-copy version.

cjus commented 7 years ago

Definitely don't recommend replacing lodash in larger applications. I'm only referring to its use in Hydra core. As an example, in Hydra we were using moment.js for a single function that was easily replaced with plain javascript.

ecwyne commented 7 years ago

@cjus @emadum Speaking of new ecmascript features, how do you guys feel about using async/await in the core of hydra?

It would mean forcing users to use Node >=7.6 The hydra-integration package already requires >=7.X

cjus commented 7 years ago

@ecwyne commit here looks good. Is it good to go?

ecwyne commented 7 years ago

@cjus Yes, it should be good to publish