Added dependency resolution modes #21

cjhoward92 · 2017-06-06T18:49:52Z

See issue #11 for details.

Examples related to transpiling can be found in the examples folder under typescript and babel.

Code coverage is ~99.62%. I cannot find the last line. coveralls was not working for me.

Regex

coveralls · 2017-06-06T18:51:41Z

Coverage decreased (-0.4%) to 99.611% when pulling 3922a67 on cjhoward92:master into 8f3ae97 on jeffijoe:master.

jeffijoe · 2017-06-06T18:54:46Z

To run the coverage, use npm run cover- npm run coveralls is used by CI.

jeffijoe · 2017-06-06T18:56:30Z

Here's the culprit: https://coveralls.io/builds/11854857/source?filename=lib%2FcreateContainer.js#L137

You sure its needed?

cjhoward92 · 2017-06-06T19:36:20Z

Ah yes. No, that method is no longer needed. I meant to remove it and just forgot. One moment.

Removed unneeded has routine

cjhoward92 · 2017-06-06T19:39:43Z

Updated. You should see 100% coverage come through here soon. Let me know if there is anything you want changed in my implementation.

coveralls · 2017-06-06T19:40:07Z

Coverage remained the same at 100.0% when pulling 098a92a on cjhoward92:master into 8f3ae97 on jeffijoe:master.

jeffijoe · 2017-06-06T20:34:04Z

I'm going to review & merge tomorrow, probably edit the readme. I want to make sure we don't overwhelm the first-time reader by explaining all the configuration methods in the introductory example. I think the new Resolution Mode deserves its own section. 😄

Nice work! 🎊

cjhoward92 · 2017-06-06T20:35:26Z

Sounds good to me! Thanks a ton! The code in there is well-written so it was an easy addition.

jeffijoe · 2017-06-06T20:36:12Z

README.md

@@ -731,7 +747,7 @@ The `['glob', Lifetime.SCOPED]` syntax is a shorthand for passing in registratio

 Clone repo, run `npm i` to install all dependencies, and then `npm run test-watch` + `npm run lint-watch` to start writing code.

-For code coverage, run `npm run coverage`.
+For code coverage, run `npm run cover`.


My bad, didn't realize I put coverage instead of cover.

It's cool. I figured I'd take care of it for you!

jeffijoe · 2017-06-06T20:39:11Z

lib/loadModules.js

@@ -39,7 +40,8 @@ module.exports = function loadModules (dependencies, globPatterns, opts) {
  opts = opts || {}
  opts = Object.assign({}, {
    registrationOptions: {
-      lifetime: Lifetime.TRANSIENT
+      lifetime: Lifetime.TRANSIENT,
+      resolutionMode: ResolutionMode.PROXY


Might want to consider pulling the default from the container. For example, by passing it into opts.

jeffijoe · 2017-06-06T20:43:39Z

lib/registrations.js

+ * The function that constructs the specified type
+ */
+function buildNewClassFunction (Type) {
+  return (dependencies) => {


If anything, you can use return function () {} and use arguments.

I am not sure why I didn't try using arguments, it ended up working way better. Thanks!

jeffijoe · 2017-06-06T20:45:11Z

lib/registrations.js

+  return function (container) {
+    // Because the container holds a global reolutionMode we need to determine it in the proper order or precedence:
+    // registration -> container -> default value
+    let resolutionMode = (this.resolutionMode || container.__resolutionMode || ResolutionModes.PROXY)


Maybe we should have a container.options where things like the default resolutionMode are used, so accessing them don't seem like a hack (the __ in the name indicates "private"). Thoughts?

I agree with having options on the container. I was thinking along those lines but you weren't storing the options on the container before so I didn't know if that would be too much. The __ was intentional so hopefully no one would use the property haha.

I will make the other changes as well.

jeffijoe · 2017-06-06T20:47:28Z

These comments I just left are things I could take care of, too - not trying to rub all the work off on you! 😄

cjhoward92 · 2017-06-06T20:49:34Z

@jeffijoe I am in a lucky position at the moment. Tomorrow is my last day at work and they basically said to "Do whatever you want" so I am doing this. I will take care of these issues! It keeps me busy and makes me feel like I am contributing to the community.

jeffijoe · 2017-06-06T20:50:41Z

That sounds great, I'll hold off on the review and edits then! Feel free to hop on the Awilix gitter, I try to be around as much as possible. I'm off for now though, thanks again!

cjhoward92 · 2017-06-06T21:01:57Z

@jeffijoe Updated for review whenever you return.

coveralls · 2017-06-06T21:02:41Z

Coverage remained the same at 100.0% when pulling 0fa6b22 on cjhoward92:master into 8f3ae97 on jeffijoe:master.

coveralls · 2017-06-06T21:05:20Z

Coverage remained the same at 100.0% when pulling b1031c1 on cjhoward92:master into 8f3ae97 on jeffijoe:master.

coveralls · 2017-06-06T21:08:45Z

Coverage remained the same at 100.0% when pulling 9f58bac on cjhoward92:master into 8f3ae97 on jeffijoe:master.

jeffijoe · 2017-06-07T10:20:03Z

lib/registrations.js

  // Use a regular function instead of an arrow function to facilitate binding to the container object
  return function (container) {
    // Because the container holds a global reolutionMode we need to determine it in the proper order or precedence:
    // registration -> container -> default value
-    let resolutionMode = (this.resolutionMode || container.__resolutionMode || ResolutionModes.PROXY)
+    let resolutionMode = (this.resolutionMode || container.options.resolutionMode || ResolutionModes.PROXY)


Not sure if this is enforced in the linter, but if you don't modify a variable after initialization, you should use const, it communicates intent and the reader will know that it won't be modified. 😄 I can fix these no problem, just letting you know.

jeffijoe · 2017-06-07T12:26:38Z

Made a few modifications + readme updates and other small things, feel free to review my commits! Released as 2.3.0 on npm.

Again, great work @cjhoward92! Are you on Twitter? I wanna give a shoutout. 😄

Carson.Howard and others added 11 commits June 5, 2017 10:28

Added manual dependency resolution

ad2ea6b

Updated README. Linted, Tested

5e4f2d6

Added 'has' comments

6e34e79

Added ResultionModes; Started refactor

b1cba1c

Finished adding resolutionMode; added TS example

3982673

Fixed regex

0ba7eee

Changed babel example output so it is ignored

9e7ff2d

Merge pull request #1 from cjhoward92/regex

fb98b55

Regex

Removed Package-lock from example

d1fe43f

updated gitignore

c4ba148

Merge pull request #2 from cjhoward92/regex

3922a67

Regex

Carson.Howard and others added 2 commits June 6, 2017 12:37

Removed unneeded has routine

5fe7595

Merge pull request #3 from cjhoward92/regex

098a92a

Removed unneeded has routine

jeffijoe reviewed Jun 6, 2017

View reviewed changes

Updated newClassFunction; added container.options

0fa6b22

Removed unnecessary comment

b1031c1

Removed buildNewClassFunction for cleaner anonymous func

9f58bac

jeffijoe reviewed Jun 7, 2017

View reviewed changes

jeffijoe merged commit 9f58bac into jeffijoe:master Jun 7, 2017

jeffijoe mentioned this pull request Dec 2, 2017

[DISCUSSION] Change default injection mode to CLASSIC? #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added dependency resolution modes #21

Added dependency resolution modes #21

cjhoward92 commented Jun 6, 2017

coveralls commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

coveralls commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

jeffijoe Jun 6, 2017

cjhoward92 Jun 6, 2017

jeffijoe Jun 6, 2017 •

edited

Loading

jeffijoe Jun 6, 2017

cjhoward92 Jun 6, 2017

jeffijoe Jun 6, 2017

cjhoward92 Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

coveralls commented Jun 6, 2017

coveralls commented Jun 6, 2017

coveralls commented Jun 6, 2017

jeffijoe Jun 7, 2017

jeffijoe commented Jun 7, 2017

Added dependency resolution modes #21

Added dependency resolution modes #21

Conversation

cjhoward92 commented Jun 6, 2017

coveralls commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

coveralls commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

jeffijoe Jun 6, 2017

Choose a reason for hiding this comment

cjhoward92 Jun 6, 2017

Choose a reason for hiding this comment

jeffijoe Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

jeffijoe Jun 6, 2017

Choose a reason for hiding this comment

cjhoward92 Jun 6, 2017

Choose a reason for hiding this comment

jeffijoe Jun 6, 2017

Choose a reason for hiding this comment

cjhoward92 Jun 6, 2017

Choose a reason for hiding this comment

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

jeffijoe commented Jun 6, 2017

cjhoward92 commented Jun 6, 2017

coveralls commented Jun 6, 2017

coveralls commented Jun 6, 2017

coveralls commented Jun 6, 2017

jeffijoe Jun 7, 2017

Choose a reason for hiding this comment

jeffijoe commented Jun 7, 2017

jeffijoe Jun 6, 2017 •

edited

Loading