[BUGFIX] Using query params helper outside of link-to

[Alonski]

The (query-params) helper has always been able to be used outside of link-to.
This PR broke it: #17779

Fixes: #18076

Recommendations:
This should be backported to the version in which #17779 was included.

Thanks a lot to @rwjblue for the help in fixing this 😄

[chancancode]

I think we need to keep this.query === UNDEFINED (and maybe assert you can’t have both)

[Alonski]

@chancancode Can you please help me understand exactly what you mean? What should the code be?

[chancancode]

if (this.query === UNDEFINED) {
  let { _models: models } = this;

  if (models.length > 0) {
    let maybeQueryParams = models[models.length - 1];

    if (isQueryParams(maybeQueryParams)) {
      this.query = maybeQueryParams.values;
      models.pop();
    }
  }
} else {
  assert(
    'Cannot pass a QueryParams object in @models and @query at the same time',
    this.models.length === 0 || isQueryParams(this.models[this.models.length - 1])
  );
}

// packages/@ember/-internals/routing/lib/system/query_params.ts

import { Option } from '@glimmer/interfaces';

export default class QueryParams {
  constructor(readonly values: Option< object> = null) {
  }

  get isQueryParams(): true {
    return true;
  }
}

export function isQueryParams(obj: unknown): obj is QueryParams {
  return typeof obj === 'object' && obj !== null && obj['isQueryParams'] === true;
}

[chancancode]

I actually don't know why the QueryParams constructor accepts null here, is that a public API?

[rwjblue]

@chancancode - In general, I agree that you shouldn't be able to set both @query and pass a (query-param as the last model, however I don't think we can check this.query === UNDEFINED unless we also change where we save off the query param value. Given the snippet you suggested above, we would only update this.query the first time (not for each subsequent didReceiveAttrs where we could have a completely new value).

[Alonski]

@chancancode I don't understand the query_params.ts snippet.
This will change the behavior of the class. Why is values not a property anymore?

[chancancode]

Okay sorry I think I understand the problem now. I know how to fix it but I'll do it later. You can revert to what you had before here (removing the this.query === UNDEFINED check and the assertion I added).

[chancancode]

@Alonski in TypeScript, constructor(readonly values: Option<object> = null) is a shorthand for declaring the field and assigning it from the constructor:

export default class QueryParams {
  constructor(readonly values: Option<object> = null) {
  }

  get isQueryParams(): true {
    return true;
  }
}

...is the same as...

export default class QueryParams {
  readonly values: Option<object>;

  constructor(values: Option<object> = null) {
    this.values = values;
  }

  get isQueryParams(): true {
    return true;
  }
}

[Alonski]

@chancancode I push some more code. Is this what you wanted?
I had to negate the assertion to get tests passing

[Alonski]

CI Failure seems like a false negative

[chancancode]

Suggested change

	if (typeof lastModel === 'object' && lastModel !== null && lastModel.isQueryParams) {
	if (isQueryParams(lastModel)) {

[Alonski]

@chancancode Reverted commit

[NullVoxPopuli]

This is exciting! Once this is merged & back ported, I can start upgrading a bunch of things to 3.12 from 3.8 :)

[chancancode]

Oops, I should have used squash and merge. Oh well.

[chancancode]

Backported to beta and release, I think we usually hold off on LTS backports until the stable release has been cut.

[Alonski]

Thanks! So this will be backported after 3.14 or 3.15 is released?

[Alonski]

@chancancode Any idea when this will be backported to 3.12?