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

Remove old code that existed for now unsupported ember-sources (2.4, etc) #1486

Merged
merged 10 commits into from
Sep 3, 2024

Conversation

NullVoxPopuli
Copy link
Sponsor Collaborator

@NullVoxPopuli NullVoxPopuli commented Aug 23, 2024

NOTE:


Not having embroider-optimized enabled has caused some bug reports related to resolving packages (@ember/-internals, @ember/renderer).

See: #1487

embroider-optimized testing was disabled in 6828d0c#diff-efa7561fd196a5304c1c80012f1f6153c58faf5697ba3a59465ae7ce06b8e550R146
(in August 2021)

This library's minimum supported ember version is 4.0: https://github.com/emberjs/ember-test-helpers
image

@NullVoxPopuli NullVoxPopuli changed the title Re-enable embroider-optimized Fix "trying to import from X but that is not one of its explicit dependencies", also, re-enable embroider-optimized testing. Aug 23, 2024
@NullVoxPopuli NullVoxPopuli changed the title Fix "trying to import from X but that is not one of its explicit dependencies", also, re-enable embroider-optimized testing. Re-enable testing against embroider-optimized Aug 23, 2024
@NullVoxPopuli NullVoxPopuli force-pushed the re-enable-embroider-optimized-testing branch from c8db6d8 to d26589e Compare August 26, 2024 18:14
this.setProperties({
jax: helper(([name]) => `${name}-jax`),
});
await render(hbs`{{this.jax "max"}}`);
Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

these helpers were (rightfully so!) trying to look up the helper defined in app/helpers/jax.

However, that doesn't exist, and since we don't actually support custom resolvers for normal apps, this is fine. These tests are testing that render can invoke helpers, and helpers can be defined on this

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review August 26, 2024 19:52
if (
EmberTest.waiters.some(([context, callback]) => !callback.call(context))
) {
return true;
}
}

return false;
return EmberTest.checkWaiters();
Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

changing this from false to EmberTest.checkWaiters() is a meaningful change, but the only mainingful change in this PR (for consumers).

The code for checkWaiters is here: https://github.com/emberjs/ember.js/blob/85a4f298f67ff70395cf7f9103682335162e0606/packages/ember-testing/lib/test/waiters.ts#L111

and those callbacks are part of the legacy waiter system.

There are existing comments about removing code for accessing legacy-waiter stuff (because ember.js has since abstracted it so we don't need to be so weird in this library) -- cleaning that up can happen in a followup PR, and should be non-breaking, and retain legacy waiter support.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

So.... I suppose this should be a patch-release, rather than non-release.

Copy link

@driesdl driesdl left a comment

Choose a reason for hiding this comment

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

To me it looks fine, but I'm not familiair with the in-depths aspects of it all.

@NullVoxPopuli NullVoxPopuli changed the title Re-enable testing against embroider-optimized Remove old code that existed for now unsupported ember-sources (2.4, etc) Aug 27, 2024
@MelSumner MelSumner requested a review from a team August 27, 2024 20:31
@simonihmig
Copy link
Contributor

We just run into an issue where tests were failing when switching to Embroider optimized (works for safe mode!) due to await settled() not capturing some pending stuff with test waiters, because those AMD requires that were still around wouldn't work. So AFAICT I think this PR should hopefully fix these cases! 🤞

@NullVoxPopuli
Copy link
Sponsor Collaborator Author

NullVoxPopuli commented Aug 29, 2024

Ye! I'm hopeful -- we need this tho:
embroider-build/embroider#2075

This test helpers pr currently contains a patch

@NullVoxPopuli
Copy link
Sponsor Collaborator Author

Updated embroider (via lockfile bumps), and things are green now without the patch!

@NullVoxPopuli NullVoxPopuli merged commit 38b7720 into master Sep 3, 2024
22 checks passed
@NullVoxPopuli NullVoxPopuli deleted the re-enable-embroider-optimized-testing branch September 3, 2024 15:21
@github-actions github-actions bot mentioned this pull request Sep 3, 2024

import EmberApplicationInstance from '@ember/application/instance';
import { Test } from 'ember-testing';
export const checkWaiters = Test.checkWaiters;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being exported? This seems to have caused a type regression, see #1502

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

great question -- I had originally thought that the original implementation was exported -- but I see in this diff that it is not. let's see if not exporting it resolves that issue

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

Successfully merging this pull request may close these issues.

5 participants