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

embroider ember-try scenarios #308

Merged
merged 3 commits into from
Aug 17, 2021
Merged

embroider ember-try scenarios #308

merged 3 commits into from
Aug 17, 2021

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Aug 16, 2021

This enables embroider-safe ember-try scenario

@stefanpenner stefanpenner marked this pull request as draft August 16, 2021 23:11
@stefanpenner stefanpenner force-pushed the embroider branch 4 times, most recently from d9aba83 to 2f58ea3 Compare August 17, 2021 10:49
@stefanpenner
Copy link
Member Author

tests pass manually, but I need to fix testem integration still.

 ERROR: Uncaught Error: Testem was unable to detect a test framework, please load it before invoking Testem.hookIntoTestFramework at http://localhost:7357/testem.js, line 913

@stefanpenner
Copy link
Member Author

Interesting, tests pass locally fine. I guess I need to dig in....

@stefanpenner
Copy link
Member Author

stefanpenner commented Aug 17, 2021

Alright QUnit works now in testem mode.

Unfortunately It looks like (in CI only as far as I can tell), we are attempting to register a webpack AMD style module (with only 1 argument) to a loader.js loader.

The error (in CI only so far) is:

Uncaught Error: an unsupported module was defined, expected `define(id, deps, module)` instead got: `1` arguments to define

@stefanpenner
Copy link
Member Author

I can reproduce the (1) the failure (2) the test timeout by running

env CI=true yarn ember try:one embroider-safe

... currently debugging

note: This fixes an embroider related issue, where when IE11 was included as a build target (in CI mode) es6-promise, babel, and webpack didn’t get along. (As described [here](embroider-build/embroider#731 (comment)))
@stefanpenner
Copy link
Member Author

stefanpenner commented Aug 17, 2021

The issue was basically: embroider-build/embroider#677 (comment) although my approach to fix it was different.

  1. opened an issue on es6-promise
  2. switched to the same promise polyfill qunit and @ember/test-helpers use
  3. rely on auto-import to handle that promise polyfill

@@ -3,21 +3,22 @@
const EmberAddon = require('ember-cli/lib/broccoli/ember-addon');

module.exports = function (defaults) {
let app = new EmberAddon(defaults, {
autoImport: {
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't need this, if we include qunit via auto-import

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the reason we had this here was that we weren't sure of broad auto-import support at the time of writing. Makes sense if it's better supported now.


app.import('node_modules/qunit/qunit/qunit.css', {
type: 'test',
});

app.import('vendor/shims/qunit.js', { type: 'test' });
Copy link
Member Author

Choose a reason for hiding this comment

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

use auto-import instead

app.import('vendor/shims/qunit.js', { type: 'test' });

return app.toTree();
try {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is verbose, because this library cannot yet drop node 10 and instead requires @embroider/test-support be provided via the ember-try-scenario configuration

@@ -85,6 +84,7 @@
"loader.js": "^4.7.0",
"npm-run-all": "^4.1.5",
"prettier": "^2.2.1",
"promise-polyfill": "^8.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. now matches @ember/test-helpers polyfill
  2. works around ie11 + es6-promise + babel + webpack "glitch" that causes some fun to debug issues.

call `Application.create(...)` on it. Additionally, applications can opt-out of this behavior by
setting `autoRun` to `false` in their `ember-cli-build.js`
*/
runningTests = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

mimic @ember/test-helpers

</div>

<script src="/testem.js" integrity=""></script>
<script src="/testem.js" integrity="" data-embroider-ignore></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

This ensures embroider doesn't warn that it can't find this, as this is a dynamic files provided by a middleware which shouldn't and cannot be bundled.

@stefanpenner stefanpenner merged commit 418c099 into master Aug 17, 2021
@stefanpenner stefanpenner deleted the embroider branch August 17, 2021 17:58
mrloop added a commit to mrloop/ember-sortable that referenced this pull request Mar 14, 2022
NullVoxPopuli pushed a commit to mrloop/ember-sortable that referenced this pull request Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants