-
-
Notifications
You must be signed in to change notification settings - Fork 154
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
Fix issues with peerDependencies when ember-auto-import present. #752
Conversation
93729d7
to
2aaef56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add a test that would have failed. Likely via ember-try
config with ember-auto-import
.
@@ -1,6 +1,8 @@ | |||
export { default as QUnitAdapter, nonTestDoneCallback } from './adapter'; | |||
export { loadTests } from './test-loader'; | |||
|
|||
import './qunit-configuration'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for side-effects (hence the somewhat odd syntax).
This makes sense to me. CI is failing though. |
2aaef56
to
aa2063f
Compare
When `ember-auto-import` is present, avoid `app.import` of `QUnit` itself. Without this change, users (when they have `peerDependencies` setup) would get an error like: ``` Uncaught Error: QUnit has already been defined. ``` This is because we were previously always `app.import`ing `QUnit` itself, but then `ember-auto-import` sees: `import { test, module } from 'qunit';` in a test file alongside the `package.json` having an entry for `qunit` so it attempts to bundle it **again**. This is the path _away_ from custom build/app.import shenanigans and helps us down the path of avoiding specialness in an Embroider focused world.
aa2063f
to
9c416df
Compare
Ya, was because I forgot to remove the |
@ef4 - FYI, this is roughly the culmination of the thing we had discussed a while back. As of 5.0 beta series (and this PR) the only |
Current failure with
|
Just pushed an update that should fix that issue with |
The `vendor/ember-cli/test-support-suffix.js` file overrides a file built into ember-cli's build pipeline and prevents this built-in `Testem.hookIntoTestFramework` invocation: https://github.com/ember-cli/ember-cli/blob/v3.20.0/lib/broccoli/test-support-suffix.js#L3-L5 The reason this override is needed, is because the built-in ember-cli invocation of `Testem.hookIntoTestFramework()` happens _during_ `test-support.js` evaluation, which (when we are leveraging Embroider or ember-auto-import) QUnit is not present (since its not eagerly evaluated, we only are guaranteed it is present when we import it). As such the `Testem.hookIntoTestFramework` invocation has been moved into our `addon-test-support/index.js` which is only ran once QUnit is actually defined/available.
369d643
to
223e0cf
Compare
/* | ||
used to determine if the application should be booted immediately when `app-name.js` is evaluated | ||
when `runningTests` the `app-name.js` file will **not** import the applications `app/app.js` and | ||
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; | ||
|
||
/* | ||
This file overrides a file built into ember-cli's build pipeline and prevents | ||
this built-in `Testem.hookIntoTestFramework` invocation: | ||
|
||
https://github.com/ember-cli/ember-cli/blob/v3.20.0/lib/broccoli/test-support-suffix.js#L3-L5 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this override is needed, is because the built-in ember-cli invocation of Testem.hookIntoTestFramework()
happens during test-support.js
evaluation, which (when we are leveraging Embroider
or ember-auto-import
) QUnit
is not present (since its not eagerly evaluated, we only are guaranteed it is present when we import it). As such the Testem.hookIntoTestFramework
invocation has been moved into our addon-test-support/index.js
which is only ran once QUnit
is actually defined/available.
} | ||
|
||
// TODO: figure out how to make this not needed, AFAICT ember-auto-import | ||
// does not provide any ability to import styles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, but we really just need to enable that in ember-auto-import, because it's allowed in the embroider spec and we want ember-auto-import to be a faithful polyfill for that.
I think keeping the app.import
here is fine until then, it's not harmful.
@@ -0,0 +1,9 @@ | |||
import QUnit from 'qunit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the app is using ember-auto-import, this line probably only works by accident.
That is, ember-auto-import won't parse this file to see that qunit is needed, because ember-auto-import is not a dependency of ember-qunit. So this import will fail at runtime, unless some other package has already caused qunit to be auto-imported.
In the common case, the app's test suite will have at least one place that imports from qunit, making this work. But it will fail very confusingly if they, say, have no tests and therefore have no place that imports qunit.
I think it would be simpler to make ember-qunit depend on ember-auto-import and always auto-import qunit. That would also allow you to delete all the code that decides whether to conditionally run app.import
.
For the compatibility case of an app that doesn't have auto-import, it would still work because the app could rely on the side effect of ember-qunit importing qunit causing it to be defined (this is the reverse of the bug I'm pointing out here -- I think it's fine if the addon always does an import and the app relies on that by side effect, whereas I don't think the addon should rely on a side effect of the app doing an import).
EDITED to add: forget what I said about side effects. They do work with ember-auto-import, but wouldn't with embroider, so we shouldn't rely on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be simpler to make ember-qunit depend on ember-auto-import and always auto-import qunit. That would also allow you to delete all the code that decides whether to conditionally run app.import.
Thank you for reviewing! I'll test that now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 257556d
Now that the addon itself depends on ember-auto-import, it no longer makes sense to test as an ember-try scenario.
This file should not be needed since emberjs#752
This file should not be needed since emberjs#752.
This file should not be needed since emberjs#752, where reference was removed. ```js this.import('vendor/ember-qunit/qunit-module.js', { type: 'test' }); ```
This file should not be needed since emberjs#752, `ember-auto-import` is used now and reference was removed.
This file should not be needed since emberjs#752, `ember-auto-import` is used now and reference was removed.
When
ember-auto-import
is present, avoidapp.import
ofQUnit
itself. Without this change, users (when they havepeerDependencies
setup) would get an error like:This is because we were previously always
app.import
ingQUnit
itself, but thenember-auto-import
sees:import { test, module } from 'qunit';
in a test file alongside thepackage.json
having an entry forqunit
so it attempts to bundle it again.This is the path away from custom build/app.import shenanigans and helps us down the path of avoiding specialness in an Embroider focused world.