fjorgemota / jimple

Just a dependency injection container to NodeJS and to the browser using new ES6 features
MIT License
75 stars 12 forks source link

Add the provider shorthand function #20

Closed homer0 closed 6 years ago

homer0 commented 6 years ago

What does this PR do?

  • This PR is the resolution of the conversation with had in #18.
  • I saw the .eslintrc file but I'm not familiar with Code Climate so I have no clue how to validate the syntax; I tried to follow your style on the files I modified.

It adds a provider shorthand function that allows the developer to create a configuration provider with a simple callback:

const { provider } = require('jimple');

module.exports = provider((container) => {
  ...
});

As I explained it on the original issue, there are a few advantages of this approach over the use of module.exports.register:

You can name your export

const { provider } = require('jimple');

...

module.exports = {
  ...
  asConfiguration: provider((container) => {
    ...
  });
}

Currying get simpler & you can have multiple configurations for the same module

const { provider } = require('jimple');

...

const asConfigurationWithOptions = (someOptions = {}) => provider((container) => {
  ...
});

const asConfiguration = asConfigurationWithOptions();

module.exports = {
  ...
  asConfigurationWithOptions,
  asConfiguration,
}

module.exports.register exists on the context of a module, while the function can be used on any context

There's no code example of this, but think of a browser environment, where modules are not fully supported yet, you could use this shorthand on anywhere, even on an AMD define.

How should it be tested manually?

npm test!

fjorgemota commented 6 years ago

Hi!

Sorry for the late answer.

What do you think about using static ES6 methods instead of adding a property provider directly in module.exports? I think the result will be more clear, more customizable (the static method as far I know can be overwritten by any class extending it) while integrating more with the project. =)

About .eslintrc, you may want to use ESLint to check if the style of the code is right. With the commit I will add soon, obviously (The file has an syntax error that i'll be fixing soon).

Thanks.

homer0 commented 6 years ago

@fjorgemota No worries :) And yes, I'll change it, it makes total sense to have it in there.

Regarding ESLint, I've been using it for a long time, but I've never seen a YAML config, and because the project doesn't have ESLint as (dev)dependency, I assumed it was being used on the integration of Code Climate.

fjorgemota commented 6 years ago

Yeah, I missed the use of ESLint a few times, but your PR will not be rejected by not using that, even with Code Climate complaining. I'll fix these problems of the project in the next release. =)

I'll await for your to change the provider to a static method so I can merge the PR.

Thanks!

homer0 commented 6 years ago

@fjorgemota quick question, I just noticed that the package.json says that the Node version should be equal or greater than v4, but on the test file you are destructuring chai, which is not available and causing an error (It seems like the last time I tested it I was on Node 6). Do you prefer I tested on a different Node version or that I change the destructuring?

fjorgemota commented 6 years ago

@homer0 Do not worry with that. The next release (2.0) will probably support only Node >= 6, so I will fix package.json later. By the way, the test matrix set on .travis.yml already includes only node >= 6

homer0 commented 6 years ago

@fjorgemota Perfect :D

fjorgemota commented 6 years ago

Thanks =)