@tracked Query Params doesn't update the URL

[chancancode]

import Controller from '@ember/controller';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';

export default class FooController extends Controller {
  queryParams = ['count'];

  @tracked count = 0;

  @action incrementTracked() {
    this.count++;
  }

  @action incrementSet() {
    this.incrementProperty('count ');
  }
}

Only incrementSet works here.

[pzuraq]

So I believe the issue here is that QPs are updated with synchronous observers, and @tracked doesn't trigger synchronous observers at all currently. It also doesn't throw an error, and I'm not sure how feasible it is to throw an error due to how observers set themselves up.

I've thought about a couple of solutions to this problem:

1. Tracked props could trigger sync observers. This solves the problem for tracked properties, and any @dependentKeyCompat properties, but could be a hit on perf and is not ideal. This is also explicitly something they don't support in the RFC.
2. Sync observers could be flushed asynchronously if they were not flushed in the last runloop. I feel like this is where most behavior will be going so this would be ideal, but it also feels like this could be unexpected - even if you explicitly declare an observer should be sync, it'll fire async if it's observing a tracked prop.
3. We could make QP observers async and force flushes internally. This would only be a temporary solution to the problem though.

I think option 2 makes sense, but we could also go with option 1, assuming we deprecate sync observers very soon.

[chancancode]

For the bug fix, can't we just refactor QPs to not use observers? We have the information available to use at instantiation time, we can do whatever we need to add setters, etc.

For the medium term (next release or two), we may be able to RFC a queryParam decorator in @ember/controller to replace the queryParams declaration. Or perhaps this is something ember-decorators could do.

[pzuraq]

For the bug fix, can't we just refactor QPs to not use observers?

This would be a pretty major refactor, and I'm not sure if it would be possible to do with the current QP semantics. The issue is that observers are used to keep QP values in sync with a cache of the values, and this cache is used to then setup the controller when the route reactivates. This cache I believe will also restore values based on dynamic segments of a given route, so there could be multiple values cached per-route.

We may be able to update these caches just before a transition, but like I said, I think this would be a pretty major refactor. Forcing a flush of async observers would probably be a better intermediate solution while we're in beta.

For the medium term (next release or two), we may be able to RFC a queryParam decorator in @ember/controller to replace the queryParams declaration.

I'd rather avoid doing this, as we already have a QP redesign API we're working on that is much more minimal and wouldn't require any annotations/declarations on either the route or the controller. ember-decorators could definitely add a QP decorator, it's been an ask for a while.

[rwjblue]

@chancancode / @pzuraq - I recall seeing this be discussed in the #st-octane channel and thought y'all came to a conclusion RE: a smallish change to fix this. Can one of you provide context (before we all forget the convo even happened) 🙃?

[pzuraq]

There are two options we discussed:

1. Dynamically wrap tracked properties with a native setter function if they are watched by query params, and have those setters trigger the same logic that observers currently do.
2. Convert the observers to async observers, and add manual async flushes during routing, like we had previously before we added sync observers back.

I think 2 is the most minimal option, and will confirm whether or not this issue is caused by this. I'm still not sure what to do in the general case for sync observers that are attempting to watch tracked properties.

[Jopie01]

I also walked into this and used set to work around this. Tested it on a clean octane app (3.14.0-canary+75c707d1).

import Controller from '@ember/controller';
import { action, set } from '@ember/object';
import { tracked } from '@glimmer/tracking';

export default class ApplicationController extends Controller {
  queryParams = ['count'];

  @tracked count = 0;
  counter = 0;

  @action incrementTracked() {
    this.counter++;
    set(this, 'count', this.counter);
  }
}

[wycats]

@pzuraq I don't consider #2 to be the more minimal option. #1 is much more targeted, and I can't see any problem with it.

In particular, changing the addObserver call to defineProperty in the case of a tracked property should "just work".

The code in question is here: https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/routing/lib/system/route.ts#L1996

[pzuraq]

The solution for #1 isn't to just replace observed properties with tracked properties. It would be to add a completely new setter on top of tracked properties, that wraps the tracked property setter.

We can't utilize autotracking here without a major refactor of the router, which is why making observers work as before would be a much smaller change overall.

Edit: That said, if anyone wants to write the code for that setter wrapping logic, I'd say go for it, it's not a massive change. The observer flushing logic could just be copied over directly from the commit before we landed async observers, and it would be maybe 4 lines of code total, which is why I believe it would be more minimal.

[wycats]

Are async observers expected to work with @tracked properties?

[pzuraq]

Yes, they were spec'd to interop, and they work correctly today

[wycats]

We can't utilize autotracking here without a major refactor of the router

Are you saying that QP getters can't work? How would you express a QP getter?

[pzuraq]

Currently, no, they definitely cannot, without @dependentKeyCompat. I don't think Godfreys solution would work here either.

[wycats]

Could we poll the QPs instead of flushing all sync observers? Where's the relevant code?

[pzuraq]

I feel like there should be a way to do this without observers at all, because the logic that is run in them is just to update the cache. Like, we should be able to update that cache just before we leave a route, so there shouldn't be a need to update it every single time its value updates.

It's just a bit of a larger refactor, and I haven't had time to dig into the way the router works, so there are some unknowns. I think it may be something we have to do though if we really want to fix this, especially for undecorated getters 😕

[chancancode]

Fixed by #18358 (for now)