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

fix($state): populate default toParams in .transitionTo. closes #1396 #1424

Closed
wants to merge 1 commit into from
Closed

fix($state): populate default toParams in .transitionTo. closes #1396 #1424

wants to merge 1 commit into from

Conversation

christopherthielen
Copy link
Contributor

No description provided.

@christopherthielen
Copy link
Contributor Author

@nateabele please review with comments from #1396

@nateabele
Copy link
Contributor

The fix is in the right spot, but you can't do a straight extend(), since way default values are assumed to be specified is only shorthand. See https://github.com/angular-ui/ui-router/blob/master/test/urlMatcherFactorySpec.js#L283

I'm still trying to decide if it'd be overkill to make a class to wrap a collection of parameters... thoughts?

@nateabele
Copy link
Contributor

Incidentally, this may actually fix a long-standing issue with uiSrefs causing a double-transition that I never fully investigated.

@christopherthielen
Copy link
Contributor Author

you can't do a straight extend()

Oh, right, I see.

I have a short laundry list of things to fix regarding typed/default parameters. I think creating a Params class makes sense, since there seem to be quite a few different ways to declare them.

Here's my laundry list:

I'm in process of (4).

  • I'd like StateBuilder's ownParams to return a map of the Params for that state (maybe as class objects, like you suggest), but that would be a behaviour change/BC. However, ownParams isn't really exposed outside of StateBuilder.
    • possibly add ownParamsKeys for current behavior, but I think this is overkill .ownParams is only used to determine the keep level using equalForKeys
  • I'd like StateBuilder params to combine parent.params with current.ownParams

@nateabele
Copy link
Contributor

  1. Typed params not allowed in query string

Yeah, currently there's no syntax for making them typed. I guess &foo:Type would work.

  1. flushTypeQueue() not called until $urlMatcherFactory is injected

Yeah, it requires $injector because type definitions are injectable. We could (a) retain a list of types in an array and just check against that, or (b) what I was going to do, which is to queue state definitions until $get, at which point the URL problem wouldn't exist.

OR, are UrlMatchers created on the fly for states? Doesn't seem so...

You mean https://github.com/angular-ui/ui-router/blob/master/src/state.js#L54?

  1. investigate Typed and optional parameters #1032 @amcdnl has issue with param not being utilized.

The current codebase has issues with user-specified capture groups (part of why I rewrote it), so it currently bails on them.

  1. URL and state params cannot be mixed arbitrarily. "Missing required parameter…" in child state with definition.params #1073 Application fails to bootstrap with two levels of dynamic params #1045

I have a feeling this is negated in the rewrite.

I'd like StateBuilder's ownParams to return a map of the Params for that state (maybe as class objects, like you suggest), but that would be a behaviour change/BC. However, ownParams isn't really exposed outside of StateBuilder.

Make it a class, and make it prototypally inherited like Scope.

@christopherthielen
Copy link
Contributor Author

Yeah, currently there's no syntax for making them typed. I guess &foo:Type would work.

How about "?{foo:Type}&{bar:int}" to get a bit closer to path syntax?

OR, are UrlMatchers created on the fly for states? Doesn't seem so...

You mean https://github.com/angular-ui/ui-router/blob/master/src/state.js#L54?

I was wondering if state.url was recomputed later, at runtime. After further review I found it was indeed computed only once at config-time.

Make it a class, and make it prototypally inherited like Scope.

You essentially want a StateParams class? I've got a Param class hooked up for individual params in a private branch. It needs a bit more loving before I push it anywhere. I'd be happy with a simple prototypal inherited map of Param objects ala Scope with string keys and Param values. Along the lines you were thinking?

@nateabele
Copy link
Contributor

How about "?{foo:Type}&{bar:int}" to get a bit closer to path syntax?

Fine either way. I was thinking just the colon would be okay since path parameters have to be wrapped in braces anyway but query parameters don't.

I was wondering if state.url was recomputed later, at runtime. After further review I found it was indeed computed only once at config-time.

Well, something will have to get recomputed at some point if we're doing an editable state tree. Maybe we can just juggle parent objects. Anyway, I think queuing state definitions until after the type queue has been flushed is the way to go.

You essentially want a StateParams class?

Right. If we have one class that takes the config hash of parameters, configure the parameter objectsand self-assign them as properties, then make the parent state's instance the prototype, parameter inheritance is solved. Make sense?

@christopherthielen christopherthielen deleted the issue-1396 branch October 8, 2014 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants