Ember.js: [Regression] [v3.10] The `(query-params)` helper can only be used when invoking the `{{link-to}}` component.

[Kerrick]

The PR #17779 introduced a backwards-incompatible change in v3.10, which should not have made it into the framework until v4.0 with deprecation warnings preceding its removal.

For many versions of Ember I've been able to do the following, but since this PR landed and v3.10 shipped, I now am presented with an error: The (query-params) helper can only be used when invoking the {{link-to}} component.

{{!-- app/templates/index.hbs --}}
{{my-component optional-params=(if foo (query-params bar="baz"))}}

{{!-- my-component.hbs --}}
{{#if optional-params}}
  {{#link-to "my.route" optional-params class="actionable"}}Schedule{{/link-to}}
{{else}}
  {{#link-to "my.route" class="passive"}}View{{/link-to}}
{{/if}}

This breaks our production application. This should be considered a breaking change. Its presence in v3.10.0 and v3.10.1 should be considered a regression, and I'd love to see a patch version v3.10.2.

[rwjblue]

The PR #17779 introduced a backwards-incompatible change in v3.10, which should not have made it into the framework until v4.0 with deprecation warnings preceding its removal.

I assure you, we understand how SemVer works.

[rwjblue]

{{!-- my-component.hbs --}}
{{#if optional-params}}
  {{#link-to "my.route" class="actionable" query-params=optional-params}}Schedule{{/link-to}}
{{else}}
  {{#link-to "my.route" class="passive"}}View{{/link-to}}
{{/if}}

@Kerrick - Can you double check that this is what you are actually using? I don't see any code that would support passing query-params as a named argument to {{link-to}} (I checked in 3.8 and 3.9). Perhaps you mean:

{{#link-to "my.route" class="actionable" optional-params}}Schedule{{/link-to}}

[Kerrick]

@Kerrick - Can you double check that this is what you are actually using? I don't see any code that would support passing query-params as a named argument to {{link-to}} (I checked in 3.8 and 3.9). > Perhaps you mean:

I was making a reduced example during a meeting and messed it up. Your correction is accurate.

I assure you, we understand how SemVer works.

If what I said came across as harsh or reductive I apologize. I tend to over-explain things to a fault. I'm sorry about that.

[NullVoxPopuli]

I ran in to this issue today when going from 3.8 -> 3.12.

in 3.8, everything is fine, but in 3.12, I got the error mentioned in the issue title.

the invocation:

{{#link-to-wrapper link='my.route' queryParams=(query-params foo='bar')}}
  Test Link
{{/link-to-wrapper}}

where link-to-wrapper is this~ish:

{{#if queryParams}}
  {{#link-to link queryParams}}
    {{yield}}
  {{/link-to}}
{{else if ...}}
...

found the commit: https://github.com/emberjs/ember.js/commit/e6c040ffe60f72c124f9698ca1712cb812655826

super curious about this, as there is even a test asserting this behavior: https://github.com/emberjs/ember.js/commit/e6c040ffe60f72c124f9698ca1712cb812655826#diff-a912b76af52c89d34aee6b496aa64b4dR54

[NullVoxPopuli]

Found another situation in which this is triggered:

trying to pass an object to a link-to for query params via

{{#link-to 'my.route' (object-to-query-params this.myObj)}}
...
{{/link-to}}

in 3.8, this worked:

export function objectToQueryParams([params]) {
  return {
    isQueryParams: true,
    values: Object.assign({}, params),
  };
}

export default Helper.helper(objectToQueryParams);

but in 3.12, I get this error:

Uncaught Error: Assertion Failed: The `(query-params)` helper can only be used when invoking the `{{link-to}}` component.

Looking at the query-params implementation, the object construction looks similar enough:
https://github.com/emberjs/ember.js/blob/e26954908bd09660dab8397a69886e64d02c9429/packages/%40ember/-internals/glimmer/lib/helpers/query-param.ts#L31-L39
QueryParams class: https://github.com/emberjs/ember.js/blob/e26954908bd09660dab8397a69886e64d02c9429/packages/%40ember/-internals/routing/lib/system/query_params.ts#L1-L7

[rwjblue]

@Kerrick (and others) - I am sorry for the rude tone in my earlier (initial) reply. I should not have taken offense at the specific phrasing in your description, and I'll try to avoid ignorant/flippant replies like that in the future. 😞

[rwjblue]

Chatted a bit about this with @Alonski in discord, and I think we have a plan of attack for fixing. Going to dump that conversation here just in case I completely forget it (👴).

Alon - Today at 1:37 PM
@rwjblue Do you think you could explain why we need this assertion?

tldr; we don't need it. We were trying to avoid having to actually have a helper named query-params and instead, transform during compilation to the angle bracket form and pass @queryParams={{hash ...}}.

The path forward here is to change this test:

https://github.com/emberjs/ember.js/blob/bc435eb0a11868f7e0f8381f9a4bb2ba7728d9e8/packages/%40ember/-internals/glimmer/tests/integration/components/link-to/query-params-curly-test.js#L71-L87

into something like:

    ['@test `(query-params)` must be used in conjunction with `{{link-to}}'](assert) {
      if (!DEBUG) {
        assert.expect(0);
        return;
      }

      this.addTemplate(
        'index',
        `{{#let (query-params foo='123') as |qp|}}{{link-to 'Index' 'index' qp}}{{/let}}`
      );

      return this.visit('/').then(() => {
        this.assertComponentElement(this.firstChild, {
          tagName: 'a',
          attrs: { href: '/', class: classMatcher('ember-view active') },
          content: 'Index',
        });
      });
    }

Then to actually fix the issue, we'd need a change in this area:

https://github.com/emberjs/ember.js/blob/755ea5dbe65d91e0d650707da740aa6900d0a755/packages/%40ember/-internals/glimmer/lib/components/link-to.ts#L841-L849

I _believe_ that we can do something like:

      let { _models: models } = this;
      if (models.length > 0) {
        let lastModel = models[models.length - 1];

        if (typeof lastModel === 'object' && lastModel !== null && lastModel.isQueryParams) {
          this.query = lastModel;
          models.pop();
        }
      }

I'm like 63% sure that is all that is needed to fix

[chancancode]

I think I thought we could _delete_ the query-params helper with the original change, but it seems like I didn't end up deleting it anyway, for some reason. Not sure why. But anyway seems good to me.

I think you also would need to pop the lastModel in the last snippet.

[rwjblue]

I think you also would need to pop the lastModel in the last snippet.

ya, good point (updated inline above)

[aaronbhansen]

On the PR related to this fix, we've hit an error when upgrading to Ember 3.12 which is:
Assertion Failed: You must use set() to set the `query` property (of <manage@component:link-to::ember1338>) to '[object Object]'

If I change one line of the code in the pr from: https://github.com/emberjs/ember.js/blob/v3.12.4/packages/%40ember/-internals/glimmer/lib/components/link-to.ts#L872

if (typeof lastModel === 'object' && lastModel !== null && lastModel.isQueryParams) {
  this.query = lastModel.values;
  models.pop();
}

to:

if (typeof lastModel === 'object' && lastModel !== null && lastModel.isQueryParams) {
  this.set('query', lastModel.values);
  models.pop();
}

The assertion error goes away and the app continues to work as expected. Any issue with changing the query to a set in the after example?

Right below this code, query is being set using set
this.set('query', params.pop().values); https://github.com/emberjs/ember.js/blob/v3.12.4/packages/%40ember/-internals/glimmer/lib/components/link-to.ts#L892

Is this something you'd patch if I opened a pr?

[Alonski]

@rwjblue I think @aaronbhansen has a point :)
My bad :)

[rwjblue]

@aaronbhansen - Can you open a new issue to track that (cross linking this PR)? I think we'd need a reproduction also. Specifically, we need to understand what is observing the query property (by default it is not observed).

[aaronbhansen]

I can open a new issue, but I'm not sure how to reproduce it outside of our code base. Locally we have link-tos that are setup the same and the error happens on some and not on others. Reproducing it was my original hesitation on opening a new bug ticket on a fresh ember build. It could be because of the complexity in our local code base and the different layers interacting. I know that when I modified ember locally with the change above it fixed it, which is why I wanted to let you know here. When I debugged it more locally, It seems to happen when the query params are re-triggered on the link-to a second time.

I was able to get around the issue by converting all of our link-to usages to angle brackets and pass the query argument in directly. That could be the solution for anyone upgrading to 3.12 if you don't feel that updating the query with a set is the right direction. If you'd still like me to open a bug ticket I can, unfortunately I wasn't to reproduce it on a new repo though. I was unsure of how to trigger the same response without re-building the complexity of what we had in a demo app.