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 Nested V2 Addons #1327

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Fix Nested V2 Addons #1327

merged 2 commits into from
Mar 7, 2023

Conversation

gossi
Copy link
Contributor

@gossi gossi commented Jan 14, 2023

Fix #1325

please refer to linked issue for detailed explanation. I didn't add any tests though, there is no such scenario in the repo. Please let me know how a good test would fit within the existing suite.

if (autoImportInstance) {
autoImportInstance.registerV2Addon(name, root);
} else {
(this.parent as EAI2Instance).registerV2Addon(name, root);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we know it's safe to call this on the parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the biggest question mark I have, quote from the issue above:

Why I'm saying this is only a suggested fix: I'm not sure, if this piece of code is in the intention of the comment or if that means a different implementation (given I have basically no real knowledge about embroider).

which basically targets this line.

But following the analysis I put in the issue, there is either a autoImportInstance or this.parent with registerV2Addon available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially, you can safeguard this. But I'd say, it would be better to throw an error over silently do nothing and not registering the addon at all, without a dev knowing about it ?!?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's safe once refactored inside the if (this.parent.pkg['ember-addon']?.version ?? 1) < 2) check.

@gossi
Copy link
Contributor Author

gossi commented Feb 5, 2023

@ef4 is this ready to move forward? It is improving and at the same time doesn't make it worse?

@ef4
Copy link
Contributor

ef4 commented Mar 6, 2023

The test coverage for this is in ember-auto-import, not embroider, because all of this registering v2 addons with ember-auto-import stuff is inert under embroider, it only matters for non-embroider apps where ember-auto-import is doing the work.

Opened embroider-build/ember-auto-import#571 to cover this.

@ef4
Copy link
Contributor

ef4 commented Mar 7, 2023

Thanks @gossi

@ef4 ef4 merged commit 8b65699 into embroider-build:main Mar 7, 2023
@wagenet
Copy link

wagenet commented Mar 27, 2023

Has this made it into a release yet?

@NullVoxPopuli
Copy link
Collaborator

yes, https://www.npmjs.com/package/@embroider/addon-shim/v/1.8.4-unstable.00ec2e7

main is released to the @unstable tag every merge.
Note, however, that unstable has no support (hence the name), and is meant as a means of testing

@njradford
Copy link

Also encountering this issue fwiw

This was referenced May 2, 2023
@ef4 ef4 added the bug Something isn't working label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read properties of undefined (reading 'registerV2Addon')
5 participants