emberjs / emberjs/ember.js

Dynamic route value for LinkTo breaks with mounted engine

Open
#18,820 1 comment 0 reactions 0 assignees View on GitHub
Engines Routing
Dominant language
TypeScript
Stars
22.6k
Forks
4.2k
Avg merge
3d 12h
Merged PRs (30d)
15

Description

## Summary

Invoking `` or `{{link-to this.aDynamicValue}}` in an Ember engine triggers an error when `aDynamicValue` is a `get`-only native computed property or a `.readOnly` classic computed property, because `route` is two-way bound and Engines updates the `route` (via public API!) to deal with namespacing.

- Original discovery at ember-engines/ember-engines#692
- Full, minimal reproduction [in this repo](https://github.com/chriskrycho/link-to-bug/)

## Details

This is the best kind of bug: the collision of a set of features which are all reasonably designed and all work independently, but when put together make for a gnarly bug!

1. When introducing support for angle bracket invocation of `LinkTo` (8261aac7), the internal value used to track the target route name was changed from `targetRouteName` to `route`—I assume so that the positional param version could just set the same value as the named param, and everything else would just work without any knowledge of the difference between the two.

2. Ember Engines [updated](https://github.com/ember-engines/ember-engines/commit/bb67f9f9b7f30d11c91982512eafa83768903676) to use the new property name in its `LinkTo` subclass, which needs to update the value of the target route. (Notably, this allowed Engines to move from using *private* API to using *public* API!) The Engines `LinkTo` subclass necessarily [prefixes route names with their mount point](https://github.com/ember-engines/ember-engines/blob/d808e6b505f215c32028f210393144468bf8d2d3/addon/components/link-to-component.js#L23) during `didReceiveAttrs`.

3. Updating the `route` value via a `set` [triggers `notifyPropertyChange`](https://github.com/emberjs/ember.js/blob/a751e2778f8c95fd053ee29c45e43a91e5e03968/packages/%40ember/-internals/metal/lib/property_set.ts#L106), as you would expect. The key for the lookup is `'route'`, and `'route'` which is the name of the argument (whether `@route=` or positional), so [looking up references in args](https://github.com/emberjs/ember.js/blob/d96d9aad52e010da977d813f479494397ee53ec6/packages/%40ember/-internals/glimmer/lib/component.ts#L781:L792) *finds* a reference and attempts to update it. I believe this will consistently fail for *any* class property (i.e. non-static-string):

- if it is a `get`-only native getter, it will trigger either browser’s early error for attempting to set a `get`-only computed property;
- if it is a `readOnly` CP, it will trigger the read-only assertion in Ember;
- and even if *neither* of those is true—i.e. if it’s just a tracked class property—it will trigger the backtracking assertion, because it will have pulled that value and then attempted to set it in the same cycle.

## Solutions?

There are a couple possible workarounds I see here after digging into this a bit with @pzuraq late last week:

- We could override `route` itself in Engines, with a set-only computed property which does not trigger the `set` behavior. This would likely work, but it seems like a hack, and one that inherently makes Engines more fragile to changes in `LinkTo` in the future.

- We could explicitly set `PROPERTY_DID_CHANGE` not to trigger further up the tree for `LinkTo` (and therefore its subclasses). This would probably work, but this *also* seems like a hack to me, and one that might have *other* unforeseen consequences?

- We could more explicitly “disconnect” the `route` value from the passed argument. In my view, this is the “right” move philosophically. For one thing, the arguments to `LinkTo` being two-way bound is extremely surprising. For another, this would make the behavior of `LinkTo` more like Glimmer components—and notably, if `LinkTo` *were* a Glimmer component, this bug wouldn’t happen, precisely because the arguments would (a) be in a different namespace and (b) would *not* be two-way bound.

While we fix it, we can also advise people to work around this by using hard-coded values with other conditions to invoke `LinkTo` statically. This is not in fact a *solution* to this bug… but it’s likely what I’ll be doing in the app where I ran into this as a short-term *workaround* for the bug.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.