code refactoring for container package #14989
Conversation
|
CI seems unhappy |
once green.
|
☔ The latest upstream changes (presumably #15014) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@homu done! |
packages/container/lib/container.js
Outdated
| @@ -330,8 +330,7 @@ function lookup(container, fullName, options = {}) { | |||
| } | |||
|
|
|||
| function isSingletonClass(container, fullName, { instantiate, singleton }) { | |||
| return (singleton !== false && isSingleton(container, fullName)) && | |||
| (instantiate === false && !shouldInstantiate(container, fullName)); | |||
| return singleton !== false && isSingleton(container, fullName) && !instantiate && !shouldInstantiate(container, fullName); | |||
| } | |||
|
|
|||
| function isSingletonInstance(container, fullName, { instantiate, singleton }) { | |||
There was a problem hiding this comment.
We typically want to avoid lines that are too long. I also feel the former is easier to read
There was a problem hiding this comment.
I was looking at this pull request. Should I replace it?
| @@ -732,31 +732,31 @@ Registry.prototype = { | |||
| getInjections(fullName) { | |||
| let injections = this._injections[fullName] || []; | |||
| if (this.fallback) { | |||
| injections = injections.concat(this.fallback.getInjections(fullName)); | |||
| injections.push(...this.fallback.getInjections(fullName)); | |||
There was a problem hiding this comment.
so you are modifying this._injections[fullName] ? , because former did not do that
There was a problem hiding this comment.
I hope you are not polluting this._injections[fullName] here :P
There was a problem hiding this comment.
before/after is not same here, so each time you call getInjections it it modifies this._injections[fullName] if there is fallback. am I wrong ? @stefanpenner @artemgurzhii
|
@stefanpenner is merging master to PR branch is right way to update with master ? |
|
@bekzod the best way is to rebase. git fetch origin
# while on your working branch
git rebase origin/master |
|
@stefanpenner just asking because this one was merged, but if it is accepted then cool |
|
☔ The latest upstream changes (presumably #15043) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #15193) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing this one due to conflicts, feel free to give it another go! |
No description provided.