Reuse iframe #8
Reuse iframe #8
Conversation
src/consumer/frame.js
Outdated
| * Cleans up references of detached nodes by setting them to null | ||
| * to avoid potential memory leak | ||
| */ | ||
| cleanup() { |
There was a problem hiding this comment.
Users may create tons of global variables that reference created frames and iframes and wrappers will be dangling in memory even after calling unmount method in old code. Though it's very unlikely to happen, this cleanup method ensures that we won't have memory leak issues even that does happen.
| super(props); | ||
|
|
||
| // Binds 'this' for methods called internally | ||
| this.handleProviderMessage = this.handleProviderMessage.bind(this); |
There was a problem hiding this comment.
Moving all of bindings out of init method. Following current ES6 coding pattern and putting them in constructor.
| * to avoid potential memory leak | ||
| */ | ||
| cleanup() { | ||
| this.iframe = null; |
There was a problem hiding this comment.
i'd probably just move this inline into the one place its used.
There was a problem hiding this comment.
yeah, that's what I did initially, but found it hard to mock setting to nulls in tests (without mocking tests will fail). And that's the reason why I created a method.
There was a problem hiding this comment.
can we at least prepend the method name with __ or move this method off the class so that its "private".
There was a problem hiding this comment.
I was kinda hesitated on this because current eslint rule (no-underscore-dangle) discourages us to use prepended underscores. Their argument is that there's no truly private members in javascript. I'm on the fence. It looks like a matter of team preference and won't affect code performance, readability, or complexity. Personally I'd like to have underscores to indicate privacy as other language does (e.g. python), however, eslint rules can be considered as somewhat official style guide of JS maybe. Any thoughts you'd like to share?
|
+1.This option will help us a lot |
| @@ -141,6 +149,26 @@ class Frame extends EventEmitter { | |||
| if (this.wrapper.parentNode === this.container) { | |||
| this.container.removeChild(this.wrapper); | |||
There was a problem hiding this comment.
After removing the wrapper do we want to set it to null?
There was a problem hiding this comment.
the wrapper is set to null in clean up method.
src/consumer/frame.js
Outdated
| */ | ||
| load(url) { | ||
| if (url) { | ||
| this.iframe.src = url; |
There was a problem hiding this comment.
Don't you have to set this.origin to the origin of the new url if they differ? I suggest move the loadPage logic into the load() method and having loadPage call this instead.
There was a problem hiding this comment.
I'd make sure we test this with loading a new src from a different origin
There was a problem hiding this comment.
Good call! I've moved the logic of loadPage to load method and had loadPage call load instead.
As for test, we do have a test page in example that shows how provider triggers an loadPage event to load a new url with a different origin (from localprovider.com to localhost). Since we've made loadPage call load method, I think that may be good to cover tests for load as well.
Summary
This PR adds a new method called
loadto frame so that user can use existing iframe to load a new page and skip the mounting step.There's also a small improvement made in this PR to avoid potential memory leak.