-
Notifications
You must be signed in to change notification settings - Fork 377
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 full embroider compatibility #1463
Conversation
8ecbb5b
to
16a0094
Compare
Would love to see this progress. 👍🏻 I was using this Webpack workaround but one of our projects uses |
16a0094
to
ce3837a
Compare
I rebased this on current master and resolved any new conflicts, should be "good" again! |
549734e
to
ce3837a
Compare
I managed to make CI a week ago, so if CI fails now I believe this time should be legitimate failures. |
ce3837a
to
715ac2f
Compare
OK, I think the failing tests should be fixed now 🤞 |
@mydea I see, so in embroider-land one can't pass a component name to the select and have the select call I assume that this is not a breaking change as it's a requirement for embroider. CI is green now! Merging it |
I noticed that CI is not running embroider scenarios. I had to comment them because CI wouldn't pass. Can you add them back to |
Yes, when passing something through like this you need to use |
Great! Merged! |
This PR hopefully makes this addon fully embroider compatible.
Note that this relies on embroider-build/embroider#922 (or something along these lines) to be merged - see embroider-build/embroider#921 (in order for embroider compatibility to really work).
This is based on #1419 in many ways, but there have been a bunch of changes in the meanwhile, so it was somewhat easier to reproduce the changes than to deal with merge conflicts etc.
I also added some tests to ensure we keep supporting overwriting the default components (which would break with certain implementations, e.g. if we imported the default components in JS-land).