Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ObservableMedia): use ObservableMedia class as provider token #158

Merged
merged 1 commit into from
Feb 7, 2017

Conversation

ThomasBurleson
Copy link
Contributor

@ThomasBurleson ThomasBurleson commented Feb 5, 2017

  • Internal refactor of BreakPoint-related classes and packaging
  • Refactor provider implementations for BreakPoints, BreakPointRegistry, & ObservableMedia
  • Improved DI with replacement of ObservableMediaService opaque token with ObservableMedia class

BREAKING CHANGE:

Remove ObservableMediaService opaque token; as it adds complexity overhead. Developers should now simply use the ObservableMedia class to inject the service instance.

-before -

import {ObservableMediaService} from '@angular/flex-layout';

@Component({...})
class MyComponent {
   constructor( @Inject(ObserverableMediaService) private media:any ) { ... }
}

-after -

import {ObservableMedia} from '@angular/flex-layout';

@Component({...})
class MyComponent {
  constructor(private media:ObservableMedia) { ... }
}

@ThomasBurleson ThomasBurleson force-pushed the wip/thomas/observable-media-interface branch 3 times, most recently from 69c3424 to e355891 Compare February 5, 2017 18:45
@ThomasBurleson ThomasBurleson added this to the v2.0.0-beta.5 milestone Feb 5, 2017
@ThomasBurleson ThomasBurleson self-assigned this Feb 5, 2017
@joshwiens
Copy link
Contributor

Context

Previous usage example doesn't have a type on the constructor prop, which throws TypeError: Cannot read property 'kind' of undefined on an AOT build in @angular/cli@1.0.0-beta.30.

Reference CLI Issue: angular/angular-cli#4427
Reproducible here: https://github.com/d3viant0ne/flex-layout-prop-type

@ThomasBurleson
Copy link
Contributor Author

@d3viant0ne - Thx to Josh for this great catch and alert.

@thekalinga
Copy link

thekalinga commented Feb 6, 2017

@ThomasBurleson

Semantically, Why does @Inject(ObservableMediaService) media return ObservableMedia instead of ObservableMediaService?

I'm not specifically asking to return an incompatible object, just that if I inject X, the type of the variable that refers this injected object to should be X or a parent of X

I see that ObservableMediaService is an opaque token & following Service sufix convention when using opaque tokens.

export const ObservableMediaService: OpaqueToken = new OpaqueToken('flex-layout-media-service');

This means, my previous comment is not fully valid.

Is this a standard practice in Angular that tokens that refer to service be named with Service as sufix for their counter part?

@ThomasBurleson
Copy link
Contributor Author

ThomasBurleson commented Feb 6, 2017

@thekalinga - when you use @Inject(<token name>) the type can be anything (if you do not explicitly specify). We created the ObservableMedia interface to provide clear usage for the injected reference.

Currently I am not aware of a standard practices for naming Tokens and injected types. I will ask the @angular core team.

@ThomasBurleson ThomasBurleson force-pushed the wip/thomas/observable-media-interface branch from e355891 to 27dc5e6 Compare February 6, 2017 17:59
@ThomasBurleson ThomasBurleson force-pushed the wip/thomas/observable-media-interface branch from 27dc5e6 to 46166f1 Compare February 6, 2017 22:18
@ThomasBurleson ThomasBurleson changed the title feat(ObservableMediaService): export ObservableMedia interface for type use feat(ObservableMedia): use ObservableMedia class as provider token Feb 6, 2017
@ThomasBurleson
Copy link
Contributor Author

@d3viant0ne - can you review plz.

*  Refactor BreakPoint-related classes.
*  Refactor implementations of Providers
*  Improve DI and replace `ObservableMediaService` opaque token with **`ObservableMedia`** class

BREAKING CHANGE:

Deprecated use of `ObservableMediaService` opaque token. Developers now simply use the ObservableMedia class to inject the service.

*before*

```js
constructor( @Inject(ObserverableMediaService) private media:any ) { ... }
```

**after**
```js
constructor(private media:ObservableMedia) { ... }
```
@ThomasBurleson ThomasBurleson force-pushed the wip/thomas/observable-media-interface branch from 46166f1 to 4b1ee6d Compare February 6, 2017 22:44
@joshwiens
Copy link
Contributor

Certainly, give me 20 minutes

@joshwiens
Copy link
Contributor

Like the DI change, Media Query api / code is nice and tidy now.

Tried you branch on my current work project, after the minor update to the constructor & it's sub, everything worked like a charm. Ship It :)

@tinayuangao tinayuangao merged commit dad69fe into master Feb 7, 2017
@ThomasBurleson ThomasBurleson deleted the wip/thomas/observable-media-interface branch February 9, 2017 14:34
karlhaas pushed a commit to karlhaas/flex-layout that referenced this pull request May 3, 2017
…ngular#158)

*  Refactor BreakPoint-related classes.
*  Refactor implementations of Providers
*  Improve DI and replace `ObservableMediaService` opaque token with **`ObservableMedia`** class

BREAKING CHANGE:

Deprecated use of `ObservableMediaService` opaque token. Developers now simply use the ObservableMedia class to inject the service.

*before*

```js
constructor( @Inject(ObserverableMediaService) private media:any ) { ... }
```

**after**
```js
constructor(private media:ObservableMedia) { ... }
```
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants