fix(loader): module.decorator order of operations is now irrelevant #14348
Conversation
|
Thanks for the PR. I do think we should add a BC note to be on the safe side. Can you please add one, that describes the new behavior, how it's different from the old behavior and how this might break existing code? Small code example (that shows how order affects execution) would also be nice. You can have a look at the Changelog or the Migration guide to see how a BC note should look. |
|
How? Amend commit 478650c and re-push? If so, won't that wipe out your comments above? |
|
Yes amend and force push |
|
The comments are independent of that. And comments on changes are also preserved if the code doesn't change |
robertmirro
force-pushed
the
rjm-issue-12382
branch
2 times, most recently
from
7a1750a to
e96c44c
Compare
|
Thanks! Please let me know if the new commit is not what you had in mind. |
|
The commit message looks great, very nice! I would only change the names And please move the Closes #... above the BC notices, so it doesn't show up in it. I'd also like to see a test that ensures defining a decorator before its provider works. |
robertmirro
force-pushed
the
rjm-issue-12382
branch
from
e96c44c to
d7de76d
Compare
|
I've made the following changes:
Regarding the additional unit test... I included the additional unit test in However, I'm not entirely convinced this test is necessary because this PR is meant to move the processing of Regarding the Neither the Contributing to AngularJS nor the AngularJS Git Commit Message Conventions documents seem to specify that the Many BC commits seem to follow this pattern: d06431e, 98c2db7, 98528be, 983b059, etc... Maybe the docs can be updated if format is critical and the above is no longer acceptable? Thanks! |
|
There's a typo in the new loaderSpec test that makes the tests fail. Wrt to the commit message guidelines, putting Closes #... before the BC notice makes the CL output prettier. But we could add that to the guidelines. |
`module.decorator` is now processed via the configBlocks order of operations and:
1. no longer throws error if declared before the provider being decorated.
2. guarantees the correct provider will be decorated when multiple, same-name
providers are defined.
(1) Prior to this fix, declaring `module.decorator` before the provider that it
decorates results in throwing an error:
```js
angular
.module('theApp', [])
.decorator('theFactory', moduleDecoratorFn)
.factory('theFactory', theFactoryFn);
// Error: [$injector:modulerr] Failed to instantiate module theApp due to:
// Error: [$injector:unpr] Unknown provider: theFactoryProvider
```
The result of this fix now allows for the declaration order above.
(2) Prior to this fix, declaring `module.decorator` before the final, same-named
provider results in that provider **not** being decorated as expected:
**NOTE:** Angular does not use provider name spacing, so the final declared
provider is selected if multiple, same-named providers are declared.
```js
angular
.module('theApp', [])
.factory('theFactory', theFactoryFn)
.decorator('theFactory', moduleDecoratorFn)
.factory('theFactory', theOtherFactoryFn);
// `theOtherFactoryFn` is selected as 'theFactory' provider but it is **not**
// decorated via `moduleDecoratorFn` as expected.
```
The result of this fix ensures that `theOtherFactoryFn` will be decorated as
expected when using the declaration order above.
Closes angular#12382
BREAKING CHANGE:
`module.decorator` declarations are now processed as part of the `module.config`
queue and may result in providers being decorated in a different order if
`module.config` blocks are also used to decorate providers via
`$provide.decorator`.
For example, consider the following declaration order in which 'theFactory' is
decorated by both a `module.decorator` and a `$provide.decorator`:
```js
angular
.module('theApp', [])
.factory('theFactory', theFactoryFn)
.config(function($provide) {
$provide.decorator('theFactory', provideDecoratorFn);
})
.decorator('theFactory', moduleDecoratorFn);
```
Prior to this fix, 'theFactory' provider would be decorated in the following
order:
1. moduleDecoratorFn
2. provideDecoratorFn
The result of this fix changes the order in which 'theFactory' is decorated
because now `module.decorator` declarations are processed in the same order as
`module.config` declarations:
1. provideDecoratorFn
2. moduleDecoratorFn
robertmirro
force-pushed
the
rjm-issue-12382
branch
from
d7de76d to
263031c
Compare
|
Oops, regarding the typo... I had been testing via I changed naming conventions that are used in the test at the last moment and then ran Unfortunately the This behavior occurs on Win7 Pro 64-bit. |
|
@robertmirro: Are you sure it "stalls". It usually takes a lot of time and CPU (and can give the impression that it's frozen), but it's not. |
|
@gkalpak I just waited 5-ish minutes. That feels "stalled" to me, but I'm open to using a different word to describe this behavior. 😄 With With |
|
That's odd (unless it just needs more time on your machine - depending on the available resources). If I were you I would (1) try giving it some more time and (2) removing |
|
Being that I'm no stranger to anomalies such as this, I'll choose option (3)... Leaving Thanks though! |
|
Yeah that would work as well, I guess 😃 |
Commit 6a2ebdb removes the caveat of declaring a provider prior to decorating it through the module.decorator function. Remove statements warning of the prior requirement for order of operations. See angular#12382, PR angular#14348, and [angular#14372 comment 206452412] (angular#14372 (comment))
Commit 6a2ebdb removes the caveat of declaring a provider prior to decorating it through the module.decorator function. Remove statements warning of the prior requirement for order of operations. See angular#12382, PR angular#14348, and [angular#14372 comment 206452412] (angular#14372 (comment))
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
See #12382
What is the new behavior (if this is a feature change)?
module.decorator declarations are now invoked via the configBlocks queue to ensure that the provider being decorated has been declared regardless of the order in which module.decorator is declared.
Does this PR introduce a breaking change?
Possibly. If the same provider is decorated multiple times via a combination of module.decorator and $provide.decorator (via module.config), the decoration order may change now depending on declaration order of the module methods.
Please check if the PR fulfills these requirements
Other information:
I didn't ngdoc the added
queueparam ofinvokeLaterAndSetModuleNamebecause this fn isn't publically exposed andqueuewasn't doc'd forinvokeLater.module.decorator() is now processed via the configBlocks order of operations and:
same-name providers are defined
Closes: #12382