URL Manager #570

NullVoxPopuli · 2020-01-04T23:25:01Z

rendered

text/0000-url-primitives.md

Co-Authored-By: Ricardo Mendes <[email protected]>

text/0000-url-primitives.md

buschtoens · 2020-01-05T05:50:33Z

text/0000-url-primitives.md

+  @service router;
+
+  fromURL(url: string): RouteInfo { // /blogs/1/posts/2?foo=bar
+    let [path, query] = url.split('?');


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.

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

Yes, they are valid: https://stackoverflow.com/a/2924187/420747

Not really common though.

legit. so, whenever someone implements this, they should def not do a naive split on '?', but split on the first occurrence of ?

text/0000-url-primitives.md

buschtoens · 2020-01-05T05:55:43Z

text/0000-url-primitives.md

+## Unresolved questions
+
+- Should the URL Manager exist as a static config or an instantiable object per-route? The above proposal is a static config / singleton, but allowing an instance per route would allow for more varied state buckets, but could also make debugging harder as there would be an URL Manager for each route segment.
+
+- `toURL` / `fromURL` or `serialize` / `deserialize`?


(Lazy-loaded) engines should be considered in this RFC as well.

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?

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 via mount(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.

ah ok, then I think everything should be compat with this.

App defines the CustomURLManager, and if needed, engines could override that per route

text/0000-url-primitives.md

buschtoens · 2020-01-05T06:02:15Z

text/0000-url-primitives.md

+    let english = 
+      path.split('/')
+          .segments.map(segment => {
+            return this.i18n.lookup(`routes.from.${segment}`, 'en-us') || segment;


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?

Wouldn't this imply that the en-us locale contains the mappings from all other languages' segments to the names used in the code?

yes? maybe? depends on how you'd want to implement it

Shouldn't this mapping actually be part of every individual locale?

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.

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.

Turbo87 · 2020-01-05T07:20:38Z

text/0000-url-primitives.md

+app and addon authors to customize the use of the URL—
+including internationalized routes, dynamic segment slugs / aliases, 
+and alternative query params behavior—though, 
+implementation of those specific things is outside the scope of *this* RFC. 


isn't that something that the Location API can already handle? 🤔

Location api can't handle nested/array query params, which is a common ask

Co-Authored-By: Jan Buschtöns <[email protected]>

gossi · 2020-01-05T17:15:58Z

text/0000-url-primitives.md

+
+- Should the URL Manager exist as a static config or an instantiable object per-route? The above proposal is a static config / singleton, but allowing an instance per route would allow for more varied state buckets, but could also make debugging harder as there would be an URL Manager for each route segment.
+
+- `toURL` / `fromURL` or `serialize` / `deserialize`?


toURL / fromURL or serialize / deserialize?

definitely toURL / fromURL - with (de)serialize you are constantly question which direction is serialize? 😇

chriskrycho

There are some really interesting ideas here. I had a couple notes/suggestions/fixes along the way, but my bigger concern is: adding further complexity to the internals of the router seems extremely high risk to me. I've just spent the last quarter dealing with pain from upgrading from 3.4 → 3.8, which included a pretty significant router internals rewrite in 3.6—and while the majority of the bugs we found and fixed were the result of our app's use of private router API (🤢), the worst of the bugs we fixed were extremely subtle issues related to changes deep in the router.

In the medium term, I'd very much like to see (and I think others would as well) Ember's router become much simpler, so we should take care that designs in this space provide a way to isolate complexity and prevent the introduction of further complexity. As such, I'm concerned by the amount of new API surface around the existing router implementation.

text/0000-url-primitives.md

chriskrycho · 2020-01-05T19:51:54Z

text/0000-url-primitives.md

+class CustomURLManager extends URLManager {
+  // Concrete example implementations are shown in the following sections.
+}


I'd just drop this entirely, given the comment here – given that all it does in this case is introduce effectively a no-op new prototype.

that's what I had initially -- this was added as a clarification by another reviewer. The examples don't duplicate the router setup. imo, for learning intent purposes, it's good to keep. Would def not recommend an empty class if this were implemented.

text/0000-url-primitives.md

chriskrycho · 2020-01-05T19:55:56Z

text/0000-url-primitives.md

+```ts
+import { module, test } from 'qunit';
+import { setupTest } from 'ember-qunit';
+
+module('Unit | Route | blogs/blog/posts/post', function(hooks) {
+  setupTest(hooks);
+
+  test('it exists', function(assert) {
+    let route = this.owner.lookup('route:blogs/blog/posts/post');
+    assert.ok(route);
+  });
+
+  test('urls are resolved', function(assert) {
+    let router = ;
+    let sampleUrl = '/blogs/1/posts/2';
+    let resultUrl = toURL(fromURL(sampleURL))
+
+    assert.equal(resultUrl, sampleUrl);
+
+    let mapInfo = router.mapInfoFor('blogs.blog.posts.post');
+
+    let sampleRouteInfo = {
+      mapInfo,
+      queryParams: {}
+      dynamicSegments: { blog: '1', post: '2' },
+    }
+    let resultRouteInfo = fromURL(toURL(sampleRouteInfo));
+
+    assert.deepEqual(resultRouteInfo, sampleRouteInfo);
+  });
+});
+```


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.

where would a test for the url-manager live? tests/unit/url-manager.js?

I agree with @chriskrycho, we should write integration and acceptance tests for them

Co-Authored-By: Chris Krycho <[email protected]>

NullVoxPopuli · 2020-01-05T20:20:59Z

but my bigger concern is: adding further complexity to the internals of the router seems extremely high risk to me.

I'm all ears for other solutions that would allow people to customize query param (de)serialization as well as control how the URL is (de)serialized in general.

This is RFC is entirely in response to comments from #380. ;)

the worst of the bugs we fixed were extremely subtle issues related to changes deep in the router.

should this RFC be implemented, all existing tests should be unchanged. :-
I think that it's possible to implement all the existing behavior using what's proposed here, and if we have gaps in tests... we'll just need to add more tests.

I'd very much like to see Ember's router become much simpler

Do you have ideas for how this could be simpler?

As such, I'm concerned by the amount of new API surface around the existing router implementation.

🤷‍♂ it's 1 extra router config option, and 2 configs per route. :-\
Doesn't seem like that big of a deal regarding API Surface. The router config already has minimal API surface. But maybe I'm missing something? idk.

chriskrycho · 2020-01-06T01:07:04Z

Don't have time or mental bandwidth to do design work, alas, but—

Doesn't seem like that big of a deal regarding API Surface. The router config already has minimal API surface. But maybe I'm missing something? idk.

It's not just that it's more API surface (though that's a concern) but that every one of these kinds of things introduces more internal implementation, and much of the router's current behavior—not least around URL resolutions, including especially for QPs—is most accurately described as "emergent and incredibly complex". Every new code path has surprising opportunities to introduce bugs there, and "just add more tests" isn't really a good answer there: there's already a ton of test coverage, but because the existing implementation has all that emergent complexity, there are many interactions which are difficult to cover exhaustively, and there are many places where the issues come down to subtle timing details. And of course: if we knew what needed more coverage, someone would write it; the problems that come up in this space are from unknown unknowns.

To be clear, I'm not trying to come crap on this design. I'm just saying that all of those concerns should be considered very high priority constraints on this and other designs in this space, given the relatively high likelihood of accidental breakage even when extreme care is taken in the design.

NullVoxPopuli · 2020-01-06T13:17:58Z

introduces more internal implementation

I was planning on replacing as much of the internal implementation with this stuff as makes sense.
Also, isn't this true of any feature? I'm confused.

is most accurately described as "emergent and incredibly complex"

that's exactly why I'm proposing some primitives for us to un-complexify URL/QP behavior.

To be clear, I'm not trying to come crap on this design.

of course ;) 'just also kind feels like FUD. :-\

chriskrycho · 2020-01-06T13:26:58Z

replacing as much of the internal implementation with this stuff as makes sense.

Frankly, then: this will cause exactly the kind of issues I’m describing. (This is not FUD; that’s a rude and foolish way to describe someone attempting to shed light on something.) I’m happy to help further clarify why and how this is risky, but I’m not making crap up; I’m highlighting a risk area that it’s perfectly fair and reasonable for you to have been unaware of. I was unaware until a few months ago! 😄

chriskrycho · 2020-01-06T13:33:59Z

Let me try to put this a different way: I think this idea or something like it is good and necessary. We can’t avoid making changes to high risk areas forever because otherwise it just gets worse and worse.

However, we may need a different strategy for how we run a transition in this area because of the high risk (especially for large apps like the one I work on!) than we need for most features. The point I’m trying to make is not “this is bad; let’s not do it” but “this is much more complicated than it appears; let’s think further about what it will take to accomplish this, possibly including very different paths to getting to the desired end state proposed”.

mehulkar · 2020-01-08T07:14:04Z

Love the idea, but echoing @chriskrycho, I would also like to see the existing router reduce complexity and expose existing abstractions for experimentation before expose introducing a new concept. I think a URLManager is a good end goal but agree that it’s high risk to try to get here before going through some intermediate steps first.

NullVoxPopuli · 2020-01-08T12:48:18Z

@mehulkar what existing stuff would you like to see exposed?

NullVoxPopuli · 2020-01-08T13:20:13Z

the apis proposed here are intended to be the low level entrypoints into anything you'd want from the router -- as is,
This is how the URL is interpreted: https://github.com/emberjs/ember.js/blob/release/packages/%40ember/-internals/routing/lib/system/router.ts#L663
This is how query params are serialized: https://github.com/emberjs/ember.js/blob/release/packages/%40ember/-internals/routing/lib/system/router.ts#L712 -- this RFC would be moving this code, along with deserialize: https://github.com/emberjs/ember.js/blob/release/packages/%40ember/-internals/routing/lib/system/router.ts#L757

atm, I don't know where the router URL gets pushed back in to the address bar.

I don't think it'd be a good idea to make these methods public, as it would make testing the URL transforming behavior more difficult if it were customized, as you'd need to setup the entire router -- and that would take you out of the realm of unit testing. :-\

mehulkar · 2020-01-10T23:19:15Z

I wrote about some ideas here: https://mehulkar.com/blog/2019/12/post-octane-ember-routing/. I'm not particularly saying the a URLManager for serialization/deserialization is bad, but more that there's no good place to put it yet, because the routing surface area is so big. I'd like to see the Router class go away entirely, for example. We don't have to agree on that particular part (and it's not part of this discussion), but my point is that I would "clean the house before adding more furniture".

NullVoxPopuli · 2020-02-07T16:26:02Z

@mehulkar

I'd like to see the Router class go away entirely, for example

what do you imagine would take its place? ideal world, no consequences

mehulkar · 2020-02-10T23:01:24Z

@NullVoxPopuli I think that post covers it, but tldr;

app/router.js can be replaced with a function that defines the route map
Application can receive the other configs (e.g. locationType) directly instead of through Router class
router:main is exposed by the container, but should be replaced with service:router.

Gaurav0 · 2020-04-07T21:42:43Z

It's not just that it's more API surface (though that's a concern) but that every one of these kinds of things introduces more internal implementation, and much of the router's current behavior—not least around URL resolutions, including especially for QPs—is most accurately described as "emergent and incredibly complex". Every new code path has surprising opportunities to introduce bugs there, and "just add more tests" isn't really a good answer there: there's already a ton of test coverage, but because the existing implementation has all that emergent complexity, there are many interactions which are difficult to cover exhaustively, and there are many places where the issues come down to subtle timing details. And of course: if we knew what needed more coverage, someone would write it; the problems that come up in this space are from unknown unknowns.

To be honest I'm not an expert on the router (@machty is the only expert), but any backward compatible way of dealing with query params is going to add more surface area to the api until we deprecate controllers. We do need to find a way forward.

NullVoxPopuli · 2021-03-14T16:39:47Z

Closing this for now due to lack of time / community interest :)
Can resurrect if there is interest

sandstrom · 2021-03-15T09:15:13Z

FWIW I think modularity around URL serialization/deserialization can be useful and liked the idea of this RFC. Sorry to hear that there wasn't enough interest.

For example, we've implemented our custom location type (https://api.emberjs.com/ember/release/classes/Location) to do locale-prefixing of routes. Basically, all routes have a /locale-code prefix, e.g. /en/about and /de/about and really just one single /about route in the router.

But by putting the locale in the route we get two versions of the same website, with different languages, plus make it easier for search engines to parse.

Obviously possible already today with custom Locations, but this seems like an even more flexible solution.

NullVoxPopuli added 5 commits October 8, 2019 21:19

begin

d40cff4

fleshing out the API

63b42f1

draft complete. need to verify existing capabilities

0a5ef00

minor updates

be32f6e

Update 0000-url-primitives.md

dfa5815

locks reviewed Jan 5, 2020

View reviewed changes

NullVoxPopuli and others added 20 commits January 4, 2020 20:50

Update text/0000-url-primitives.md

52e241f

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

c1c96c5

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

f20547a

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

dfb1e31

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

900723c

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

68d32a8

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

487bbfe

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

5fe3edd

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

d4cdc5d

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

a7ed088

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

453b7a2

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

c0d093c

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

0e5b192

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

40c23a9

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

b9b80dd

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

fb2027b

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

0774eee

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

6823d79

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

ea16383

Co-Authored-By: Ricardo Mendes <[email protected]>

Update text/0000-url-primitives.md

0764081

Co-Authored-By: Ricardo Mendes <[email protected]>

buschtoens reviewed Jan 5, 2020

View reviewed changes

Turbo87 reviewed Jan 5, 2020

View reviewed changes

NullVoxPopuli and others added 2 commits January 5, 2020 09:10

Update text/0000-url-primitives.md

b4578bb

Co-Authored-By: Jan Buschtöns <[email protected]>

Update text/0000-url-primitives.md

0d06b04

Co-Authored-By: Jan Buschtöns <[email protected]>

Update 0000-url-primitives.md

1eeaab6

gossi reviewed Jan 5, 2020

View reviewed changes

chriskrycho reviewed Jan 5, 2020

View reviewed changes

NullVoxPopuli and others added 2 commits January 5, 2020 15:11

Update text/0000-url-primitives.md

d3aa12b

Co-Authored-By: Chris Krycho <[email protected]>

Update text/0000-url-primitives.md

1cabcb2

Co-Authored-By: Chris Krycho <[email protected]>

Update 0000-url-primitives.md

bfd243d

jayjayjpg mentioned this pull request Jan 10, 2020

The Ember Times No. 131 - January 17th 2020 ember-learn/ember-blog#458

Merged

28 tasks

amyrlam mentioned this pull request Jan 17, 2020

The Ember Times No. 132 - January 24th 2020 ember-learn/ember-blog#498

Merged

25 tasks

amyrlam mentioned this pull request Jan 24, 2020

The Ember Times No. 133 - January 31st 2020 ember-learn/ember-blog#505

Merged

27 tasks

NullVoxPopuli mentioned this pull request Feb 8, 2020

An alternative to Controllers #499

Closed

NullVoxPopuli closed this Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

URL Manager #570

URL Manager #570

NullVoxPopuli commented Jan 4, 2020 •

edited

Loading

buschtoens Jan 5, 2020

NullVoxPopuli Jan 5, 2020

buschtoens Jan 5, 2020

NullVoxPopuli Jan 5, 2020

buschtoens Jan 5, 2020

NullVoxPopuli Jan 5, 2020

buschtoens Jan 5, 2020

NullVoxPopuli Jan 5, 2020

buschtoens Jan 5, 2020

NullVoxPopuli Jan 5, 2020 •

edited

Loading

buschtoens Jan 5, 2020

Turbo87 Jan 5, 2020

NullVoxPopuli Jan 5, 2020 •

edited

Loading

gossi Jan 5, 2020

chriskrycho left a comment

chriskrycho Jan 5, 2020

NullVoxPopuli Jan 5, 2020

chriskrycho Jan 5, 2020

NullVoxPopuli Jan 5, 2020

villander Jan 10, 2020

NullVoxPopuli commented Jan 5, 2020 •

edited

Loading

chriskrycho commented Jan 6, 2020

NullVoxPopuli commented Jan 6, 2020

chriskrycho commented Jan 6, 2020 •

edited

Loading

chriskrycho commented Jan 6, 2020

mehulkar commented Jan 8, 2020 •

edited

Loading

NullVoxPopuli commented Jan 8, 2020

NullVoxPopuli commented Jan 8, 2020

mehulkar commented Jan 10, 2020 •

edited

Loading

NullVoxPopuli commented Feb 7, 2020 •

edited

Loading

mehulkar commented Feb 10, 2020 •

edited

Loading

Gaurav0 commented Apr 7, 2020

NullVoxPopuli commented Mar 14, 2021

sandstrom commented Mar 15, 2021


		- Should the URL Manager exist as a static config or an instantiable object per-route? The above proposal is a static config / singleton, but allowing an instance per route would allow for more varied state buckets, but could also make debugging harder as there would be an URL Manager for each route segment.

		- `toURL` / `fromURL` or `serialize` / `deserialize`?

URL Manager #570

URL Manager #570

Conversation

NullVoxPopuli commented Jan 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Jan 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Jan 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Jan 5, 2020 • edited Loading

chriskrycho commented Jan 6, 2020

NullVoxPopuli commented Jan 6, 2020

chriskrycho commented Jan 6, 2020 • edited Loading

chriskrycho commented Jan 6, 2020

mehulkar commented Jan 8, 2020 • edited Loading

NullVoxPopuli commented Jan 8, 2020

NullVoxPopuli commented Jan 8, 2020

mehulkar commented Jan 10, 2020 • edited Loading

NullVoxPopuli commented Feb 7, 2020 • edited Loading

mehulkar commented Feb 10, 2020 • edited Loading

Gaurav0 commented Apr 7, 2020

NullVoxPopuli commented Mar 14, 2021

sandstrom commented Mar 15, 2021

NullVoxPopuli commented Jan 4, 2020 •

edited

Loading

NullVoxPopuli Jan 5, 2020 •

edited

Loading

NullVoxPopuli Jan 5, 2020 •

edited

Loading

NullVoxPopuli commented Jan 5, 2020 •

edited

Loading

chriskrycho commented Jan 6, 2020 •

edited

Loading

mehulkar commented Jan 8, 2020 •

edited

Loading

mehulkar commented Jan 10, 2020 •

edited

Loading

NullVoxPopuli commented Feb 7, 2020 •

edited

Loading

mehulkar commented Feb 10, 2020 •

edited

Loading