Updated: "Fix element not calling attach internals"#142
Merged
calebdwilliams merged 11 commits intocalebdwilliams:mainfrom Apr 16, 2025
Merged
Conversation
needed to avoid error thrown in cases were internals are attached in the upgradeInternals function
Circular dependencies dist/utils.js -> dist/mutation-observers.js -> dist/aom.js -> dist/utils.js dist/utils.js -> dist/mutation-observers.js -> dist/utils.js
calebdwilliams
requested changes
Apr 16, 2025
Owner
calebdwilliams
left a comment
calebdwilliams
left a comment
There was a problem hiding this comment.
A couple small change requests, but overall this looks great!
Also another huge shoutout to @christophe-g. I haven't been the most responsive on my open source stuff lately (trying to protect myself from burn out) so I'm really sorry I didn't get to your PR in time.
calebdwilliams
approved these changes
Apr 16, 2025
Owner
calebdwilliams
left a comment
calebdwilliams
left a comment
There was a problem hiding this comment.
lgtm; thanks @Cliffback
Owner
|
This has been released as |
Contributor
Author
|
Awesome! Thanks for the quick feedback, approval and release, this is much appreciated! And thanks to @christophe-g for the original fix! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Due to inactivity on #130 that fixes #127, which we need in our project, I forked the original PR and implemented the requested changes, hopefully so that it can get merged soon.
To address @calebdwilliams comment regarding removing the guard against a second instance, this didn't work out for us, as we need the upgradeInternals to be called. I therefore added a flag to prevent the error being thrown if the attach internals function is run in upgradeInternals.
I also fixed some rollup warnings, like circular dependencies, undefined this, and missing name argument.
All credits to @christophe-g for the original implementation. I mean no disrespect by implementing his change, and would be more than happy to close this / change it to only fix the rollup warnings if the original PR gets merged.