-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add compatibility with ember-qunit@5 #816
Add compatibility with ember-qunit@5 #816
Conversation
Looks good so far. Thanks for implementing explicit calls vs. side-effect imports! |
What's the status of this PR, @rwjblue? Can we publish for review, or are you still poking at it? Also, publishing |
I still have a few TODO's that need to be completed.
Good idea! Created ember-cli/ember-cli#9335 with the TODOs. |
1178cf8
to
6f72500
Compare
6f72500
to
ce47497
Compare
Just did a rebase to fix merge conflicts. |
ce47497
to
5bd9841
Compare
https://github.com/rwjblue/--ember-qunit-5-sample-app is a demo app that uses the "whole new setup" and is passing. |
In terms of the API we expose to users, I'd prefer that we use the pattern that requires the hosting project to pass in import { setup } from "qunit-dom";
// ...
setup(QUnit); This will not require any special configuration from Thoughts? |
Ya, agreed. That's what the current implementation does (though I need to push a commit to the demo repo above with that change in it). |
In the PR description, these two are the same thing, no?
|
@rwjblue I updated the description to better reflect the actual final work for this PR. |
6a2b0c3
to
a036016
Compare
f74dda9
to
cbc763c
Compare
Begins splitting things apart to be more compatible with not assuming QUnit is am ambient global.
* Add scenarios for ember-qunit@5 * Add scenarios for embroider * Add new scenarios to CI
c742b5f
to
4d03063
Compare
4d03063
to
1909bb4
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.
LGTM after these minor comments are resolved. nice work! 👍
1909bb4
to
4e24695
Compare
Now that we have moved to requiring the callsite to pass in `QUnit.assert` to `setup` we no longer need `ember-auto-import` as a dependency.
4e24695
to
caf1b36
Compare
In order to add support for
ember-qunit@5
the primary Ember project usage pattern changes a bit:No change is required for ember-qunit < 5 (this should not be a breaking change).
Closes #812