Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX Release] fix router test regression for isActive #19789

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Oct 14, 2021

draft PR for now, I need to verify on some internal code

Continuation of #19733

kategengler and others added 30 commits May 3, 2021 11:56
The recently modules API update means we are now loading real modules,
not polyfills based on the global. This means that the modules
themselves are _eagerly required_, rather than being references to a
value on the global. For example, previously, this:

```js
import { registerWaiter } from '@ember/test';

if (someCondition) {
  registerWaiter(() => {});
}
```

Would become this:

```js
if (someCondition) {
  Ember.Test.registerWaiter(() => {});
}
```

In either example, `registerWaiter` may or may not be called based on
the state of `someCondition`. However, in the second case, if
`Ember.Test` is not defined at all, it's completely ok as long as
`someCondition` is `false`. It's never called, so we never get an error
telling us `Ember.Test` is undefined.

Without the transform, the module is eagerly required, along with all of
its dependencies. If no one included `ember-testing`, then that means
it will throw an error immediately.

This PR makes the `@ember/test` module load `ember-testing` lazily, and
if it's not available (e.g. in a production environment) it replaces the
values with a function that throws a helpful error.
* Unify deprecation ids
* Add missing `url` and `since`
* Correctly reference `@ember/legacy-built-in-components` addon
* Error instead for `<LinkTo @href=...>`
(cherry picked from commit 8e57edd)
Makes the hash helper's individual keys lazy so that they do not eagerly
incur costs.

(cherry picked from commit 7d334cf)
Makes all {{hash}} object properties settable, but deprecates setting
them.

(cherry picked from commit ec4c034)
Example message:

```
Usage of the Ember Global is deprecated. You should import the Ember module or the specific API instead.

See https://deprecations.emberjs.com/v3.x/#toc_ember-global for details.

Usages of the Ember Global may be caused by an outdated ember-cli-babel dependency. The following steps may help:

* Upgrade your `devDependencies` on `ember-cli-babel` to `^7.26.6`.
* Upgrade the following addons to the latest version:
  * active-model-adapter
  * ember-animated
  * ember-async-await-helper
  * ember-attacher
  * ember-cli-showdown
  * ember-freestyle
  * ember-md5

### Important ###

In order to avoid repeatedly showing the same deprecation messages, no further deprecation messages will be shown for usages of the Ember Global until ember-cli-babel is upgraded to v7.26.6 or above.

To see all instances of this deprecation message at runtime, set the `EMBER_GLOBAL_DEPRECATIONS` environment variable to "all", e.g. `EMBER_GLOBAL_DEPRECATIONS=all ember test`.

### Details ###

Prior to v7.26.6, ember-cli-babel sometimes transpiled imports into the equivalent Ember Global API, potentially triggering this deprecation message even when you did not directly reference the Ember Global.

The following outdated versions are found in your project:

* [email protected], currently used by:
  * [email protected]
    * Depends on [email protected]

* [email protected], currently used by:
  * [email protected]
    * Depends on ember-cli-babel@^6.8.2
  * [email protected]
    * Depends on ember-cli-babel@^6.17.0
    * Added by [email protected]
  * [email protected]
    * Depends on ember-cli-babel@^6.16.0
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.16.0
  * [email protected]
    * Depends on ember-cli-babel@^6.8.1
  * [email protected]
    * Depends on ember-cli-babel@^6.0.0
    * Added by [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.0.0-beta.4
    * Added by [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.6.0
    * Added by [email protected]
  * [email protected]
    * Depends on ember-cli-babel@^6.6.0
    * Added by [email protected] > [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.9.0
    * Added by [email protected]

* [email protected], currently used by:
  * @embroider/[email protected] (Compatible)
    * Depends on ember-cli-babel@^7.23.0
    * Added by [email protected]
    * Added by [email protected]
  * direwolf (your app)
    * Depends on [email protected]
  * [email protected] (Compatible)
    * Depends on ember-cli-babel@^7.23.0
  * [email protected] (Compatible)
    * Depends on ember-cli-babel@^7.23.0
  * [email protected] (Compatible)
    * Depends on ember-cli-babel@^7.23.0
  * [email protected] (Compatible)
    * Depends on ember-cli-babel@^7.23.0
    * Added by [email protected]
    * Added by [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^7.23.0
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^7.23.0

Note: Addons marked as "Dormant" does not appear to have any JavaScript files. Therefore, even if they are using an old version ember-cli-babel, they are unlikely to be the cuplrit of this deprecation and can likely be ignored.

Note: Addons marked as "Compatible" are already compatible with [email protected]. Try upgrading your `devDependencies` on `ember-cli-babel` to `^7.26.6`.

```

(cherry picked from commit bb9d96e)
Show details about outdated ember-cli-babel and only show the first instance by default.

Example message:

```
Using `computed.reads` has been deprecated. Instead, import the value directly from @ember/object/computed:

import { reads } from '@ember/object/computed';

These usages may be caused by an outdated ember-cli-babel dependency. The following steps may help:

* Upgrade the following addons to the latest version:
  * active-model-adapter
  * ember-animated
  * ember-async-await-helper
  * ember-attacher
  * ember-cli-showdown
  * ember-md5

### Important ###

In order to avoid repeatedly showing the same deprecation messages, no further deprecation messages will be shown for theses deprecated usages until ember-cli-babel is upgraded to v7.26.6 or above.

To see all instances of this deprecation message, set the `EMBER_RUNLOOP_AND_COMPUTED_DOT_ACCESS_DEPRECATIONS` environment variable to "all", e.g. `EMBER_RUNLOOP_AND_COMPUTED_DOT_ACCESS_DEPRECATIONS=all ember test`.

### Details ###

Prior to v7.26.6, ember-cli-babel sometimes transpiled imports into the equivalent Ember Global API, potentially triggering this deprecation message indirectly, even when you did not observe these deprecated usages in your code.

The following outdated versions are found in your project:

* [email protected], currently used by:
  * [email protected]
    * Depends on [email protected]

* [email protected], currently used by:
  * [email protected]
    * Depends on ember-cli-babel@^6.8.2
  * [email protected]
    * Depends on ember-cli-babel@^6.17.0
    * Added by [email protected]
  * [email protected]
    * Depends on ember-cli-babel@^6.16.0
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.16.0
  * [email protected]
    * Depends on ember-cli-babel@^6.8.1
  * [email protected]
    * Depends on ember-cli-babel@^6.0.0
    * Added by [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.0.0-beta.4
    * Added by [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.6.0
    * Added by [email protected]
  * [email protected]
    * Depends on ember-cli-babel@^6.6.0
    * Added by [email protected] > [email protected]
  * [email protected] (Dormant)
    * Depends on ember-cli-babel@^6.9.0
    * Added by [email protected]

Note: Addons marked as "Dormant" does not appear to have any JavaScript files. Therefore, even if they are using an old version ember-cli-babel, they are unlikely to be the cuplrit of this deprecation and can likely be ignored.
```

(cherry picked from commit 7a8ab95)
(cherry picked from commit 72c3fd7)
(cherry picked from commit 6706b17)
Ensures that computeds can depend on dynamic hash keys that did not
exist on the original hash.

(cherry picked from commit 628a928)
(cherry picked from commit 926452f)
Fixes a few things:

* Ensures that Embroider has a consistent protocol for ensuring the global is bootstrapped
* Allows our override code to be transpiled
* Ensures that any bootstrapping tweaks force us out of "prebuilt" mode (allowing that customized bootstrap code to actually run)

Refactors the work in emberjs#19557 to address some of the recent comments there.

(cherry picked from commit 5ad6f79)
chriskrycho and others added 2 commits August 30, 2021 16:37
As part of the improvements made between 3.24 and 3.28, the router
microlib is now lazily loaded. When these changes were made, there were
a couple cases where it *should* be possible to access router state in
a non-application test (integration etc.) but it currently is not
because the router is not necessarily set up. Since `setupRouter` is
idempotent, call it in those functions so that if it is *not* set up,
it gets set up, and otherwise it will continue working as expected.

(cherry picked from commit 9f61c7a)
@NullVoxPopuli NullVoxPopuli changed the base branch from master to release October 14, 2021 13:40
@NullVoxPopuli NullVoxPopuli changed the title [BUGFIX LTS] fix router test regression for isActive [BUGFIX Release] fix router test regression for isActive Oct 14, 2021
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a bit more info here, and likely a failing test case that emulates your scenario.

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Oct 22, 2021

Yeah, I haven't actually had a chance to confirm if this works, because in my app I'm getting this error:

Compile Error: <:default> is not a component and doesn't support block parameters

it points at a file doing:

<SomeComponent>
  <:default>
    {{!-- ... --}}

But I haven't yet been able to reproduce :(
(not that I've been able to put a lot of energy into getting to the bottom of it*)

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Oct 22, 2021

Well, rebasing to the latest 3.28 got me passed that. (3.28.2)

@rwjblue, I'll add a test as soon as I can confirm that this is worth finishing

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Oct 22, 2021

My current error is

Build Error (broccoli-persistent-filter:Babel > [Babel: @ember/test-helpers]) in @ember/test-helpers/setup-rendering-context.js

<my-app>/@ember/test-helpers/setup-rendering-context.js: Cannot read property 'indexOf' of null

I know this is unrelated, but I think it important to document this stuff, and I don't know where else atm.

@NullVoxPopuli
Copy link
Contributor Author

nay, this does not fix my issue, even after rebasing on top of 3.28.4 hmm

@NullVoxPopuli
Copy link
Contributor Author

Well, this seems unrelated as I'm still getting:

  TypeError: Cannot set properties of null (setting 'localName')
                at Object.<anonymous> (https://localhost:60050/search/assets/tests.js:2295:49)

when trying to access the currentRoute on the router service

@NullVoxPopuli
Copy link
Contributor Author

oh no, they've stubbed the router...

@NullVoxPopuli
Copy link
Contributor Author

I was gonna close this for now, because I seem to not need it now that I removed the code that was stubbing the router 🙃

But, I was thinking that if the router is ever not setup, wouldn't it be better to set it up in the _router getter?

I've pushed up an example.
This way all accessed properties setup the router lazily.

@@ -511,7 +509,9 @@ RouterService.reopen(Evented, {
@type String
@public
*/
currentRouteName: readOnly('_router.currentRouteName'),
get currentRouteName() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an unrelated change -- I can revert this if the approach currently committed is desired (while I add tests)

@NullVoxPopuli
Copy link
Contributor Author

Gonna close this for now -- it's been too long and I don't remember what was going on -- will submit a new PR if I encounter this again when I go back to trying some 3.25 -> 3.28 upgrades

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.