Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

module.decorator() requires explicit order of operations. #12382

Closed
bennadel opened this issue Jul 20, 2015 · 3 comments
Closed

module.decorator() requires explicit order of operations. #12382

bennadel opened this issue Jul 20, 2015 · 3 comments

Comments

@bennadel
Copy link

All of the other module-level API methods - service, factory, run, config, etc - can execute in any arbitrary order. However, the new .decorator() method has to run after the target service is defined. This is because the feature has been merely piped into the $provide.decorator() method, which used to be required to run in the configuration phase, after all services have been defined. Now that it can be called at any time, the necessary $get() method doesn't always exist.

As such, this fails:

angular.module( "Foo" ).decorator( "greet", fn ); // error, greetProvider cannot be found.
angular.module( "Foo" ).service( "greet", fn );

... but this works:

angular.module( "Foo" ).service( "greet", fn );
angular.module( "Foo" ).decorator( "greet", fn );

If you could just queue up the .decorator() calls and run them as the first part of the config-queue-flush, I think this would solve the dependency on order-of-operations.

I don't know much about contributing to Angular, so in the future, hopefully I can submit this as part of a PR.

@Narretz
Copy link
Contributor

Narretz commented Aug 3, 2015

Hi @bennadel feel free to send a PR. There are a few guidelines here: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#submitting-a-pull-request

@mchlbrnd
Copy link

Hi @bennadel,

Regarding your proposed fix of moving .decorator() up the queue; I tried decorating a provider which got injected into the config block without the decoration. See http://plnkr.co/edit/d7wMrcv8cwxQfwo6C0SC?p=preview. Line 22 in scripts.js throws because the decorator (added function to FooProvider) can't be found.

Not sure whether moving .decorator() up like you proposed will also solve the issue for providers. Can you elaborate on this, and (hopefully) on its status? :)

If you need help, do let me know!

@robertmirro
Copy link
Contributor

@bennadel

Another issue with the module.decorator() order of operations seems to be related to multiple, same-named providers (potentially an issue when mocks are used) where Angular uses the last/final declared provider.

For example, Angular uses theOtherProviderFn when injecting 'theProvider', however it is not decorated because .decorator() is declared first:

angular
    .module('theApp', [])
    .provider('theProvider', theProviderFn)
    .decorator('theProvider', theProviderDecoratorFn)
    .provider('theProvider', theOtherProviderFn);

However, decoration would occur as expected when .decorator() is declared after all providers:

angular
    .module('theApp', [])
    .provider('theProvider', theProviderFn)
    .provider('theProvider', theOtherProviderFn)
    .decorator('theProvider', theProviderDecoratorFn);

robertmirro pushed a commit to robertmirro/angular.js that referenced this issue Mar 30, 2016
module.decorator() is now processed via the configBlocks order of
operations and:
- no longer throws if declared before the provider being decorated
- guarantees the correct provider will be decorated when multiple,
  same-name providers are defined

Closes: angular#12382
robertmirro pushed a commit to robertmirro/angular.js that referenced this issue Apr 8, 2016
`module.decorator` is now processed via the configBlocks order of operations and:
  1. no longer throws 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', theDecoratorFn)
  .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', theDecoratorFn)
  .factory('theFactory', theOtherFactoryFn);

// `theOtherFactoryFn` is selected as 'theFactory' provider but it is **not** decorated
// via `theDecoratorFn` as expected
```

The result of this fix ensures that `theOtherFactoryFn` will be decorated as expected when
using the declaration order above.

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', anotherDecoratorFn);
  })
  .decorator('theFactory', theDecoratorFn);
```

Prior to this fix, 'theFactory' provider would be decorated in the following order:
  1. theDecoratorFn
  2. anotherDecoratorFn

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. anotherDecoratorFn
  2. theDecoratorFn

Closes angular#12382
robertmirro pushed a commit to robertmirro/angular.js that referenced this issue Apr 8, 2016
`module.decorator` is now processed via the configBlocks order of operations and:
  1. no longer throws 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', theDecoratorFn)
  .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', theDecoratorFn)
  .factory('theFactory', theOtherFactoryFn);

// `theOtherFactoryFn` is selected as 'theFactory' provider but it is **not**
// decorated via `theDecoratorFn` as expected
```

The result of this fix ensures that `theOtherFactoryFn` will be decorated as
expected when using the declaration order above.

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', anotherDecoratorFn);
  })
  .decorator('theFactory', theDecoratorFn);
```

Prior to this fix, 'theFactory' provider would be decorated in the following
order:
  1. theDecoratorFn
  2. anotherDecoratorFn

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. anotherDecoratorFn
  2. theDecoratorFn

Closes angular#12382
robertmirro pushed a commit to robertmirro/angular.js that referenced this issue May 1, 2016
`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 pushed a commit to robertmirro/angular.js that referenced this issue May 6, 2016
`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
sjbarker added a commit to sjbarker/angular.js that referenced this issue May 31, 2016
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))
sjbarker added a commit to sjbarker/angular.js that referenced this issue May 31, 2016
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))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants