Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
URL Manager #570
URL Manager #570
Changes from 5 commits
d40cff4
63b42f1
0a5ef00
be32f6e
dfa5815
52e241f
c1c96c5
f20547a
dfb1e31
900723c
68d32a8
487bbfe
5fe3edd
d4cdc5d
a7ed088
453b7a2
c0d093c
0e5b192
40c23a9
b9b80dd
fb2027b
0774eee
6823d79
ea16383
0764081
b4578bb
0d06b04
1eeaab6
d3aa12b
1cabcb2
bfd243d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that something that the
Location
API can already handle? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Location api can't handle nested/array query params, which is a common ask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge case, but this would fail for a URL including more than one
?
like:https://example.com/foo?param?otherParam
You could use the
URL
, however it's not natively available in IE11 and may be significantly slower than plain string processing. It could also be faster though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are multiple
?
valid? wouldn't you need to escape additional?
?fwiw, I'm not proposing that these examples be actual implementation. They need tests, and there are def cases not covered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are valid: https://stackoverflow.com/a/2924187/420747
Not really common though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
legit. so, whenever someone implements this, they should def not do a naive split on '?', but split on the first occurrence of
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this imply that the
en-us
locale contains the mappings from all other languages' segments to the names used in the code? Shouldn't this mapping actually be part of every individual locale?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes? maybe? depends on how you'd want to implement it
maybe. I don't know how molt people would want to implement this. Ember's route's are typically named in english, hence the approach here.
With this RFC, I mostly just want to enable people to play around with this, and I hope that someone does a much better job with i18n routes than I have in this example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to note, that if used like this, it'd probably be wiser to remove the
en-us
in favor of the current locale.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience, route unit tests are almost always a very bad thing. Granted that this might change the dynamics around that somewhat, but it also seems to me that what is being tested should not usually be for individual routes but instead for (a) the custom manager class and/or (b) the extended router class.
Note that even in your test here, you're not actually testing the route, you're testing how the router handles the definition of that route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where would a test for the url-manager live?
tests/unit/url-manager.js
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @chriskrycho, we should write integration and acceptance tests for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Lazy-loaded) engines should be considered in this RFC as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is engine support totally in addon space? I know nothing of engines? how would URL manipulation affect engines?
or is it more just that there could be multiple routers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Engine support is in core and addon space. Routable engines partake in the routing process. They define their routing map in
routes.js
and are mounted in the parent routing map viamount(name: string, options?: MountOptions)
.The engine routing maps are basically merged with the host routing map — it's a bit more complicated — but engines don't have their own
Router
instances.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, then I think everything should be compat with this.
App defines the CustomURLManager, and if needed, engines could override that per route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely
toURL
/fromURL
- with(de)serialize
you are constantly question which direction is serialize? 😇