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.

Sign up for GitHub

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

Jump to bottom

[router]: using @inject for ROUTE_DATA #4392

Closed
cexbrayat opened this issue Sep 28, 2015 · 8 comments
Closed

[router]: using @inject for ROUTE_DATA #4392

cexbrayat opened this issue Sep 28, 2015 · 8 comments
Labels
breaking changes effort1: hours help wanted An issue that is suitable for a community contributor (based on its complexity/scope). refactoring Issue that involves refactoring or code-cleanup
Milestone
beta.0

Comments

@cexbrayat
Copy link
Member

Don't you think that having to use @Inject for ROUTE_DATA is a bit surprising compared to RouteParams which gives us a service? I think this is a very rare case (the only one?) where we can't rely on Type annotation in an app for something provided by the framework. And we obtain an object, whereas RouteParams provides a get method.

constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
  let id = routeParams.get('id');
  let name = ROUTE_DATA.name;
}

A less surprising (and more coherent and friendly in my opinion) would be:

constructor(routeParams: RouteParams, routeData: RouteData) {
  let id = routeParams.get('id');
  let name = routeData.get('name');
}

(this is a follow up issue from #3541)
@cexbrayat
Copy link
Member Author

cc @btford @TommyM @PascalPrecht

@PatrickJS
Copy link
Member

The injector allows for any token include types. I agree with @cexbrayat that we should allow the type token RouteData on the merit that it's a valid token. I guess naming conventions should also have ref in the name RouteParamsRef and RouteDataRef. The real problem here is that ROUTE_DATA can only guarantee a shallow copy (since only two types are allowed array|object) as it's unaware of the types you provide. Unless data is a factory

@RouteConfig([
  {
    path: '/metadata',
    component: MetaDataCmp,
    data() {
      return new AwesomeData('wat');
    }
  }
])

if not RouteData than perhaps RouteConfigRef

constructor(routeParams: RouteParams, routeConfigRef: RouteConfigRef) {
  let id = routeParams.get('id');
  let name = routeConfigRef.get('data').name;
}

or perhaps the bindings in router-outlet is missing bind(ComponentInstruction).toValue(nextInstruction) and RouteConfig needs to include bindings prop. Adding these two seems to be the best option since anyone can create their own conventions

@mhevery
Copy link
Contributor

mhevery commented Sep 29, 2015

👍 for having RouteData type so that it can be cleanly injected.

@mhevery mhevery added comp: router breaking changes help wanted An issue that is suitable for a community contributor (based on its complexity/scope). refactoring Issue that involves refactoring or code-cleanup effort1: hours labels Sep 29, 2015
@btford
Copy link
Contributor

btford commented Sep 29, 2015

I think I'm convinced a type would be better.

PR welcome :)

@cexbrayat
Copy link
Member Author

@mhevery @btford Thx for your answers.

Do we agree on a RouteData type that we can inject, and which would offer a getter (as RouteParams does)?

constructor(routeParams: RouteParams, routeData: RouteData) {
  let id = routeParams.get('id');
  let isAdmin = routeData.get('isAdmin');
}

If so, I guess a Route data should be a simple object, like:

new Route({path: '/user/:id', component: UserCmp, data: { isAdmin: true }})

Would that be OK?

cexbrayat added a commit to cexbrayat/angular that referenced this issue Sep 30, 2015
@cexbrayat
refactor(router): RouteData as a type
1c6df4f 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Sep 30, 2015
@cexbrayat
refactor(router): RouteData as a type
76a995d 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
@cexbrayat cexbrayat mentioned this issue Sep 30, 2015
Closed
@cexbrayat
Copy link
Member Author

(I opened a related PR #4428 to start the discussion, feedback welcome)

@0x-r4bbit
Copy link
Contributor

@cexbrayat thanks for starting this, I already took a look and it LVGTM!

cexbrayat added a commit to cexbrayat/angular that referenced this issue Sep 30, 2015
@cexbrayat
refactor(router): RouteData as a type
7e7dcc5 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
9dd7bef 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
3772377 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
cac0bd7 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
8b76795 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
97bbbf2 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
741dfbb 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
7c89e17 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
76b61d5 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
f67608c 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
d9c243a 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
cexbrayat added a commit to cexbrayat/angular that referenced this issue Oct 9, 2015
@cexbrayat
refactor(router): RouteData as a type
bc7bac9 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes angular#4392
@mhevery mhevery added this to the Beta milestone Oct 27, 2015
@mhevery mhevery added this to the Beta milestone Oct 27, 2015
btford pushed a commit that referenced this issue Oct 27, 2015
@cexbrayat @btford
refactor(router): RouteData as a type
ff3e6cf 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes #4392
btford pushed a commit that referenced this issue Oct 27, 2015
@cexbrayat @btford
refactor(router): RouteData as a type
f130c30 
BREAKING CHANGE

The ROUTE_DATA token has been removed and replaced with a type RouteData,
allowing a type injection like we do with RouteParams.

Before:

    constructor(routeParams: RouteParams, @Inject(ROUTE_DATA) routeData) {
      let id = routeParams.get('id');
      let name = ROUTE_DATA.name;
    }

After:

    constructor(routeParams: RouteParams, routeData: RouteData) {
      let id = routeParams.get('id');
      let name = routeData.get('name');
    }

Fixes #4392
@btford btford closed this as completed in b87da8f Oct 27, 2015
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
breaking changes effort1: hours help wanted An issue that is suitable for a community contributor (based on its complexity/scope). refactoring Issue that involves refactoring or code-cleanup
Projects
None yet
Milestone
beta.0
Development

Successfully merging a pull request may close this issue.

refactor(router): RouteData as a type cexbrayat/angular
5 participants
@mhevery @cexbrayat @0x-r4bbit @btford @PatrickJS