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 close release destroyObserver #1229

Closed
wants to merge 3 commits into from

Conversation

aniude
Copy link
Contributor

@aniude aniude commented Apr 10, 2024

No description provided.

Copy link
Owner

@davidjbradshaw davidjbradshaw left a comment

Choose a reason for hiding this comment

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

Good spot, not sure what happened there. Interested to know how you spotted it. Was it causing an issue?

@@ -68,7 +68,8 @@
onScroll: function () {
return true
}
})
}),
destroyObserver
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please move this up into the alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@aniude
Copy link
Contributor Author

aniude commented Apr 15, 2024

Good spot, not sure what happened there. Interested to know how you spotted it. Was it causing an issue?

I found that in Chrome memory watcher, when I destroy the iframe element, this object still get reference to frame object as DetachIFrameObject

@davidjbradshaw
Copy link
Owner

davidjbradshaw commented Apr 16, 2024

Just tried to merge this but I get the following eslint errors

/Users/davidbradshaw/dev/iframe-resizer/src/iframeResizer.js
   733:5  error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1152:9  error  Function 'createDestroyObserver' expected no return value              consistent-return

@davidjbradshaw
Copy link
Owner

@aniude I think the first issue can be fixed by changing

 destroyObserver && destroyObserver.disconnect()

to

destroyObserver?.disconnect()

Not sure what is causing the second warning

@aniude
Copy link
Contributor Author

aniude commented Apr 16, 2024

@aniude I think the first issue can be fixed by changing

 destroyObserver && destroyObserver.disconnect()

to

destroyObserver?.disconnect()

Not sure what is causing the second warning

I changed it. check it again.

@davidjbradshaw
Copy link
Owner

Merged into V4 branch and released as v4.3.10 Thanks for the fix.

Btw you might want to checkout the version 5 I'm currently working. Draft docs can be found at http://iframe-resizer.com. Would love any feedback on it that you might have.

@aniude
Copy link
Contributor Author

aniude commented Apr 19, 2024

Merged into V4 branch and released as v4.3.10 Thanks for the fix.

Btw you might want to checkout the version 5 I'm currently working. Draft docs can be found at http://iframe-resizer.com. Would love any feedback on it that you might have.

It seems that the UI is more better. I can't wait anymore. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants