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

Fix embroider compatibility #542

Merged
merged 1 commit into from
Nov 16, 2019
Merged

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Nov 14, 2019

This package defines an empty module for the convenience of typescript users, so they can say import "qunit-dom" to get the types loaded. It's not really needed for any actual code, because the code gets applied directly to QUnit's own objects.

But because this package declares itself an ember-addon, its own module namespace is expected to be defined by real modules in treeForAddon or treeForAddonTestSupport. Not a dynamic define in vendor.

define() is most commonly used for things that aren't in an addon's own module namespace, and that continues to work under embroider. But this more unusual case -- an addon define()ing itself -- is not supported.

Without this change, attempting to import "qunit-dom" fails under embroider.

vendor/dummy-module.js Outdated Show resolved Hide resolved
@ef4
Copy link
Contributor Author

ef4 commented Nov 14, 2019

I changed this so the dummy module is expressed as named AMD. This requires me to be more nuanced about what embroider supports, since I just said above that an addon can't define() its own modules, and it looks like my PR is doing that.

To be more precise, an addon can't define() its own modules from other arbitrary files (like vendor/define-dummy-module.js). But we do support the case where we're handed a treeForAddon or treeForAddonTestSupport which happens to have been precompiled to named AMD. This case works because the files are all still in their correctly resolvable locations, and the names that are being defined() precisely match the real filesystem layout.

vendor/dummy-module.js Outdated Show resolved Hide resolved
This package defines an empty module for the convenience of typescript users, so they can say `import "qunit-dom"` to get the types loaded. It's not really needed for any actual code, because the code gets applied directly to QUnit's own objects.

But because this package declares itself an ember-addon, its own module namespace is expected to be defined by real modules in `treeForAddon` or `treeForAddonTestSupport`. Not a dynamic `define` in `vendor`.

`define` is most commonly used for things that aren't in an addons own module namespace, and that continues to work under embroider. But this more unusual case -- an addon `define`ing itself -- is not supported.

Without this change, attempting to `import "qunit-dom"` fails under embroider.
@ef4 ef4 force-pushed the embroider-compat branch from 2f8f9fd to 3fc3c41 Compare November 14, 2019 14:19
@Turbo87 Turbo87 changed the title Expose a real module Fix embroider compatibility Nov 14, 2019
@Turbo87 Turbo87 merged commit 2a73e56 into mainmatter:master Nov 16, 2019
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.

2 participants