-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Description
While working on ember-engines/ember-engines#642 to make ember-engines compatible with Ember Octane, I noticed that #17772 appears to have broken an edge case for <LinkTo> without a @route argument, but with a @query argument.
I made a reproduction here: https://codesandbox.io/s/p32ozvvz0x
If no explicit @route is passed it will be set to (or remain) UNDEFINED:
ember.js/packages/@ember/-internals/glimmer/lib/components/link-to.ts
Lines 895 to 900 in 2cc3573
| // 3. If there is a `route`, it is now at index 0. | |
| if (params.length === 0) { | |
| this.set('route', UNDEFINED); | |
| } else { | |
| this.set('route', params.shift()); | |
| } |
In this case _route returns the currently active route:
ember.js/packages/@ember/-internals/glimmer/lib/components/link-to.ts
Lines 490 to 493 in 2cc3573
| _route: computed('route', '_currentRoute', function computeLinkToComponentRoute(this: any) { | |
| let { route } = this; | |
| return route === UNDEFINED ? this._currentRoute : route; | |
| }), |
This then breaks the href computed property in L791, if the router performs an intermediate transition to an error route:
ember.js/packages/@ember/-internals/glimmer/lib/components/link-to.ts
Lines 749 to 804 in 2cc3573
| /** | |
| Sets the element's `href` attribute to the url for | |
| the `LinkComponent`'s targeted route. | |
| If the `LinkComponent`'s `tagName` is changed to a value other | |
| than `a`, this property will be ignored. | |
| @property href | |
| @private | |
| */ | |
| href: computed( | |
| '_currentRouterState', | |
| '_route', | |
| '_models', | |
| '_query', | |
| 'tagName', | |
| 'loading', | |
| 'loadingHref', | |
| function computeLinkToComponentHref(this: any) { | |
| if (this.tagName !== 'a') { | |
| return; | |
| } | |
| if (this.loading) { | |
| return this.loadingHref; | |
| } | |
| let { _route: route, _models: models, _query: query, _routing: routing } = this; | |
| if (DEBUG) { | |
| /* | |
| * Unfortunately, to get decent error messages, we need to do this. | |
| * In some future state we should be able to use a "feature flag" | |
| * which allows us to strip this without needing to call it twice. | |
| * | |
| * if (isDebugBuild()) { | |
| * // Do the useful debug thing, probably including try/catch. | |
| * } else { | |
| * // Do the performant thing. | |
| * } | |
| */ | |
| try { | |
| return routing.generateURL(route, models, query); | |
| } catch (e) { | |
| // tslint:disable-next-line:max-line-length | |
| assert( | |
| `You attempted to generate a link for the "${this.route}" route, but did not ` + | |
| `pass the models required for generating its dynamic segments. ` + | |
| e.message | |
| ); | |
| } | |
| } else { | |
| return routing.generateURL(route, models, query); | |
| } | |
| } | |
| ), |
routing.generateURL(route, models, query) is called as:
routing.generateURL("blog.post.error", [], { lang: "Japanese" });Which misses the error param:
Error: You must provide param `error` to `generate`.
This apparently used to work and while being a weird edge-case, I would still expect it to work.