-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Typed and optional parameters #1032
Conversation
[BREAK] This is a breaking change: state parameters are no longer automatically coerced to strings, and unspecified parameter values are now set to undefined rather than null.
Implements optional parameters and default parameter values. [BC-BREAK]: the `params` option in state configurations must now be an object keyed by parameter name.
$urlRouter's listener can now be made to take lower priority than custom listeners by calling `$urlRouterProvider.deferIntercept()` to detach the listener, and `$urlRouter.listen()` to reattach it.
Implements strict (and non-strict) matching, for configuring whether UrlMatchers should treat URLs with and without trailing slashes identically.
urlMatcherFactoryProvider.isMatcher() now validates the entire UrlMatcher interface.
When defining parameter configurations, `{ param: "default" }` is now equivalent to `{ param: { value: "default" }}`.
Adds documentation for the Type object, and misc. cleanup and improvement of UrlMatcher and $urlMatcherFactory docs.
Correctly detects injectable functions annotated as arrays when registering new type definitions.
This PR is a call for comments before I move onto the next steps, which are dynamic/observable parameters, moving the rest of template/ cc: @legomind, @stereokai, @jshado1, @timcharper, @kevinrenskers, @timkindberg, @ProLoser |
This is awesome. Params optional by default? 👍 |
@SimpleAsCouldBe To be clear, params are not optional by default, they're only made that way when assigned a default value. |
Hmmm. Why not always optional, and leave it up to app code to manage? Breaking change reasons? I guess as long as I can default it to undefined, I'm all good. What's with the extra layer of
|
I don't understand what this means. It's up to app code to manage either way, but if all parameters are optional, and I have to check to make sure something is not null or undefined every time I want to use it, that sounds like a massive pain in the ass.
See bullet point 2.
Yes.
See 5b72430. |
Friggin Awesome 🍻 I will review in more detail this weekend. |
Outstanding work Nate.
This breaks a lot of common sense and brings back questions from the original discussion, which are solved in this PR, just for example: How do you tackle slashes when there are multiple optional parameters in a given url, and a value is only passed to the latter? @nateabele Great work on this one. I could find only one place in the specs, where you handle null values. In my implementation - all optional parameters are null by default - but it seemed in your tests they are populated with consecutive numbers (1, 2, 3, 4, etc). The whole purpose in oppams for our project was that we could have URLs like these:
That was my guideline when working on my solutions - have you considered anything like this? Cheers! |
@crisbeto in your route you specify |
@crisbeto I'm not sure why you would expect a default parameter value to redirect to a different URL...? |
@nateabele I was expecting it to "fill" out the missing place in the URL, but I suppose that wasn't the intended functionality in the first place and could would be worked around. |
OK, I got an interesting case. I'm trying to forward an old URL to the new one. My state looks something like: So the URL is: /topics/:id?start&end $urlRouterProvider.when('/shows/:id', '/topics/:id') I left out start and end since they were optional, and since they were a querystring, I was hoping they would just be copied over if there. Sadly when I go to the previous URL with the optional params, it removes them... but I'm assuming if I set the optional params in the .when() ... then if they are not there, it won't catch it. Any ideas? |
Not sure, never tried that. Maybe look at the docs for |
I think I have a solution. Our setup now has all optional params in the query string. So, I should be able to write a relatively simple regex that looks for /shows/:id[?.*] (been lazy on my regex's, so will have to do a quick reference to write the correct one to look for an option '?' followed by anything. |
@bobmash I don't think(know) if you can pass parameters using a regex capture group using Also, because using |
@bobmash Did you try |
I found a new bug around this today. If I have the following routes:
and then I do:
Result: things work fine, but if I do:
despite reportId being defined as null, I still get: Result: which resolves to the first route not my 'detail' route. If I set |
@amcdnl Please open a new issue with a Plunkr that demonstrates it. Thanks. |
This is awesome. |
Are optional params not allowed on abstract states? $stateProvider
.state('root', {
abstract: true,
url: '/:locale',
params: {
locale: [
'$preferences',
function setLocaleParam($preference) {
var locale = $preference.get('locale');
return locale;
}
]
}
}); urlMatcherFactory.js#L509 does not even get called with the above (but it does for non-abstract states). |
Ah, it seems optional params cannot be set on an abstract parent state: jsfiddle (comment out |
I ran into the same problem as @jshado1 . I'd really like to see support for param inheritance. |
@amcdnl was the issue of it retaining parameters when using a go transition ever raised as another bug, as we are seeing the same thing |
@WayneFurmidge Ugh, I forgot :( ... you can log your case |
are any of you successfully using Typed params? I think there is a bug that stops typed param registry from working at all. |
I see alot of "see the docs for more info & examples" but am not seeing any of this information in the docs anywhere. In fact, I didn't even know about any of this until I saw this PR.
I'm not sure where the docs are generated, the ngdocs branch of ui-router says 0.2.8 |
After updating to 12 and 13, I'm getting unmatched params on the following route:
when I goto a url like:
was there an API change in that version that I missed? |
@intellix I updated the docs for 0.2.12 release |
@christopherthielen I'm expecting appId to be there and recordId to be undefined. Would the same still apply? |
@amcdnl Ah, I now see you have a trailing slash on your URL. I think that pattern should be matching that URL. Open a new issue, please. That looks like a bug, if it's not matching |
@christopherthielen Ah, your right. #1576 |
I have overhauled the URL-matching system
Here's a run-down of the new goodies and various other relevant notes:
params
is no longer mutually exclusive withurl
, and instead of being an array, it is now an object where the keys are parameter names and the values are the parameter's configurationparams: { foo: { value: null } }
makes thefoo
parameter optional in a URL — changingnull
to, for example,"bar"
, will populate$stateParams.foo
with"bar"
when no value is supplied (see the tests for extended examples)$urlMatcherFactoryProvider.caseInsensitiveMatch()
is now$urlMatcherFactoryProvider.caseInsensitive()
, and the second parameter ofUrlMatcher
's constructor is an object hash rather than a booleanUrlMatcher
s to match URLs with or without a trailing slash — see$urlMatcherFactoryProvider.strictMode()
$urlRouter
— this isn't directly related, but I snuck it in anyway: users now have the ability to delay$urlRouter
from binding to$locationChangeSuccess
, allowing the injection of custom behavior prior to syncing the state machine to the current URL.. see the docs for more info