Added the possibility to set an id to the iframe #25

ylesaout · 2019-01-15T17:12:08Z

For some reasons, I need to have an id on the created iframe. Thus this PR.

Aaronius · 2019-01-15T17:49:31Z

Have you tried:

const connection = Penpal.connectToChild({
  // URL of page to load into iframe.
  url: 'http://example.com/iframe.html',
  // Container to which the iframe should be appended.
  appendTo: document.getElementById('iframeContainer'),
  // Methods parent is exposing to child
  methods: {
    add(num1, num2) {
      return num1 + num2;
    }
  }
});

connection.iframe.id = 'foo';

If you have, can you describe why it's not working for your use case?

ylesaout · 2019-01-15T20:44:08Z

Sorry, I read the doc to quickly and didn't notice that the iframe element is returned.
I will try tomorrow and will close the PR if it works.
Thank you.

ylesaout · 2019-01-16T23:06:59Z

After some tests, using the iframe element returned by connectToChild is not usable. Indeed, when I try to get the iframe name from inside the iframe, it works on Firefox but not on Chrome. Below you will find the content of 2 files, just open parent.html in your browser.

parent.html

<!DOCTYPE html>
<html>

<head>
    <script src="https://unpkg.com/penpal/dist/penpal.min.js"></script>
    <script type="text/javascript">
        function createChilds() {
            var foo = Penpal.connectToChild({
                url: './child.html'
            });
            foo.iframe.id = 'foo';

            var bar = document.createElement('iframe');
            bar.id = 'bar';
            bar.src = './child.html'
            document.body.appendChild(bar);
        }

        window.addEventListener("load", createChilds);
    </script>
</head>

<body></body>

</html>

child.html

<!DOCTYPE html>
<html>

<head>
    <script src="https://unpkg.com/penpal/dist/penpal.min.js"></script>
    <script type="text/javascript">
        function displayName() {
            var div = document.createElement('div');
            div.textContent = window.name || "window name not set";
            document.body.appendChild(div);
        }
        window.addEventListener("load", displayName);
    </script>
</head>

<body></body>

</html>

I have an almost done update of the PR, which adds a new method Penpal.createChildConnection. This methods take the same args as Penpal.connectToChild and return the iframe, the destroy method and a connect method. The promise is returned when calling the connect method.
While the current use will still be functionnal, you will be able to also something like that in the parent:

import Penpal from 'penpal';

const connection = Penpal.createChildConnection({
  // URL of page to load into iframe.
  url: 'http://example.com/iframe.html',
  // Container to which the iframe should be appended.
  appendTo: document.getElementById('iframeContainer'),
  // Methods parent is exposing to child
  methods: {
    add(num1, num2) {
      return num1 + num2;
    }
  }
});

connection.connect().then(child => {
  child.multiply(2, 6).then(total => console.log(total));
  child.divide(12, 4).then(total => console.log(total));
});

Please note that the current PR by itself is useless as it allows only to set the iframe id and not the iframe name (the browser I'm targeting, an old webkit based one, derives the iframe name from the id if the name is not set. It looks like this is not working like that in more recent browsers).

Aaronius · 2019-01-17T00:14:32Z

Thanks @ylesaout. I want to avoid opening up a new method. Let me do some research on the id/name stuff and get back to you.

ylesaout · 2019-01-17T15:24:26Z

Another solution would be to add the possibility to pass an iframe element in the connectToChild method. Thus you would be able to call this method with an url, in which case Penpal will create the iframe element and append it to the DOM, or an iframe, in which case Penpal will only append the iframe to the DOM.

By the way, getting a pointer to the iframe element only after it has been added to the DOM causes some troubles. Not only for my case with id/name but also for sandbox attribute which I believe is more important to take into account (see this PR #10).
With Firefox 64 on Ubuntu, security is broken when setting the sandbox attribute after the iframe has been appended to the DOM. Below you'll find the test case.

parent.html

<!DOCTYPE html>
<html>
<head>
    <script src="https://unpkg.com/penpal/dist/penpal.min.js"></script>
    <script type="text/javascript">
        function shouldNotWork() {
            var div = document.createElement('div');
            div.textContent = "I shouldn't have been displayed";
            document.body.appendChild(div);          
        }
        function createChild() {
            var connection = Penpal.connectToChild({
                url: './child.html'
            });
            connection.iframe.sandbox = "allow-scripts";
            
        }
        window.addEventListener("load", createChild);
    </script>
</head>
<body></body>
</html>

child.html

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript">
        parent.shouldNotWork();
    </script>
</head>
<body></body>
</html>

Aaronius · 2019-01-18T05:28:52Z

Good find. Yeah, the security issue is certainly something that needs to be addressed. I agree with your proposal. Specifically:

Add an option named iframe to the options object passed to connectToChild(). The iframe option is optional. If provided, the iframe will be used; otherwise, an iframe will be created internally as it is currently.

What I'm on the fence about is whether to make the url option optional. If it were optional, the consumer could set the URL directly on the iframe instead of providing it in the options object. I don't think that would cause any issues, considering the iframe won't have been added to the DOM yet (adding the iframe to the DOM before passing it to connectToChild would still be prohibited). The consumer would have to either set the URL on the iframe or pass the URL in on the options object. Do you have an opinion on the matter? I'm kind of leaning toward keeping URL required on the options object since I don't think there's much benefit it providing it on the iframe instead and it keeps the API consistent.

If you're willing to help out, I would appreciate any contribution to make this happen. We'll need updates to the source, documentation, tests, and the TypeScript typings. I'm not very familiar with TypeScript so I've largely depended on the other contributors for related help.

Thanks again for doing the detective work on this and the help you've provided thus far.

ylesaout · 2019-01-18T17:21:05Z

You're welcome and thanks for the feedback and quick responses.
I hope I will be able to push you an updated PR at the beginning of next week (with tests, sources, types and docs).

About the URL parameter I don't have any opinion on either it should be optional or mandatory. So I will follow yours for the implementation.

In the case that the iframe is already happened to the DOM, should the connectToChild method throw directly an error or do you prefer that the error to be thrown through the returned promise ? Or any other idea on what should be the behavior in this case ?

Aaronius · 2019-01-18T18:52:55Z

In the case that the iframe is already happened to the DOM, should the connectToChild method throw directly an error or do you prefer that the error to be thrown through the returned promise ? Or any other idea on what should be the behavior in this case ?

The approach I would probably take is if the iframe is already added to the DOM, just remove it from the DOM before adding event listeners, setting the URL, and adding it back to the DOM. Then, in the docs for the iframe option, I'd just mention that the iframe shouldn't be added to the DOM prior to it being passed in. If we take this approach and they do happen to have it added to the DOM before passing the iframe to Penpal, I don't think anything will behave improperly, but it just wouldn't be a fully supported or encouraged use case. Does that make sense?

ylesaout · 2019-01-18T22:29:52Z

Yes it makes sense, but it depends on how you want your lib to behave. Do you prefer having a lib that throws errors early or that fails silently and leads to unexpected behavior ?

For example, imagine I have added the iframe to the DOM (but not at document.body) and I call connectToChild without appendTo option. Should the lib remove and append the iframe to document.body or to the current iframe parent element ? If I have I provide a different appenTo element than the current iframe parent element, should the lib append the iframe to the appenTo element or the current parent iframe element ?

Another case, if I call connectToChild with an iframe that has a different src than the url provided in the call. Shoudl the lib replace the src attribute or not ?

As I'm the guy that will add your lib in our code base, it doesn't matter. I have read the doc (more or less ;)) and, now, I hope I know exactly what I'm doing.

However, other people will have to maintain the code I produce and they, surely, won't read the doc. They will update the code and it will work. Until the day there will have a guy that will append the iframe to the DOM before calling penpal ! And still it will work ! And some months later, another guy need to fix the code triggered by the iframe load event and notice that the event is triggered twice (if the lib removes and appends the iframe). If it's a smart guy he will search why the load event is triggered twice, and will read the doc, ..., otherwise it will just ignore the first load event and fix the bug by running the code only on the second event. And so on ... (FYI I have to maintain production code that checks, when an iframe in inserted in the DOM, that the URL is different than "about:blank", -- of course, synchronously, it is not-- and call a recursive setTimeout...)

Thus, in my POV, it would be better not to change the current API and to add another function which takes an iframe instead of an URL. And this new method must raise an error if the iframe is already inserted in the DOM. With this API, the lib doesn't have to make opinionated choices.

Aaronius · 2019-01-18T23:33:01Z

From your last comment, it sounds like, at least as a consumer, you have a preference toward throwing an error. Let's go with throwing an error if the iframe is already added to the DOM or src has already been set. Let's not add a different method though.

These would have been my answers to your specific questions if we didn't throw an error:

Should the lib remove and append the iframe to document.body or to the current iframe parent element?

document.body

If I have I provide a different appenTo element than the current iframe parent element, should the lib append the iframe to the appenTo element or the current parent iframe element ?

The appendTo element.

Another case, if I call connectToChild with an iframe that has a different src than the url provided in the call. Shoudl the lib replace the src attribute or not ?

Replace the URL.

ylesaout · 2019-01-19T02:00:16Z

Indeed, I have a preference toward throwing an error. It's just that, by experience, if some libs had been more strict on their usage, lot of the bugs I have to fix won't have even been possible (people, at least on the project I'm working on currently, have preferred to rely on workarounds instead of finding and fixing the root cause).

BTW, as you agree on throwing error, that's good in my POV and there is no need for a new method. I'll push an updated PR in the comming days.

Note: As you may have noticed, english is not my native language. Reading the thread I may appear a little bit rude, but that's not my intention, I don't master all the nicety of english. Sorry for that!
Nevertheless I want you to thank you for how attentive you are about this PR and to know that your lib is just the one that fits most of my need: kudos to have forked postmate 2 years ago and still maintening your fork. Your PR dollarshaveclub/postmate#15 is still open and will maybe merge in the coming months. This PR (and especially your 3 points at the top of the PR thread) makes me look at penpal ;)

Aaronius · 2019-01-19T22:44:35Z

Hey, no problem! Your English is fine. I appreciate you being willing to help out and express your opinion. Let me know if you have questions or need help along the way. Thank you for the kind words about Penpal as well. We use it on some of our projects at work and it's worked out well for us.

ylesaout · 2019-01-21T17:18:51Z

Hi,
I just pushed an update with source code, types and tests updated. Let me know if you have any comments on it.
It remains me the documentation to update.

Aaronius

It's looking good! Thanks! I left a few comments.

Aaronius · 2019-01-22T00:06:27Z

types/index.d.ts

@@ -55,6 +55,7 @@ export interface IConnectionOptions {
 export interface IChildConnectionOptions extends IConnectionOptions {
  url: string;
  appendTo?: HTMLElement;
+  iframe?: HTMLIFrameElement;


It looks like ERR_IFRAME_ALREADY_ATTACHED_TO_DOM needs to be added to this file as well.

You're right, it's done.

I think it needs to be added at the bottom of the file like the other error constants as well.

Indeed, I have done the job to quickly (and as you, I'm not really familiar with TypeScript).
So I added it also to the export interface PenpalStatic and as export const ERR_IFRAME_ALREADY_ATTACHED_TO_DOM, like the other constants.

Aaronius · 2019-01-22T00:09:14Z

test/index.js

-        add: (num1, num2) => {
-          return num1 + num2;
-        }
+    it('throws ERR_IFRAME_ALREADY_ATTACHED_TO_DOM error of the iframe is already attached to DOM', () => {


Typo: should be "if the iframe"

Aaronius · 2019-01-22T00:10:12Z

test/index.js

-      url: `${CHILD_SERVER}/child.html`,
-      appendTo: container
+
+    it('creates an iframe and add it to a specific element', () => {


Both this test and the one before it should say "adds it to" instead of "add it to". That's my fault though. Don't feel like you have to fix it, but if you do, that'd be great.

Fixed as well

Aaronius · 2019-01-23T17:10:07Z

I can help write the documentation if you'd like. Let me know.

ylesaout · 2019-01-23T21:18:27Z

As you know, help is always welcome ;). I just pushed a new commit with a first version of the documentation. So if you have any comments or improvements, I'll be be happy to update the documentation.

Aaronius · 2019-01-24T00:08:56Z

Excellent. Thank you so much for the contribution. For the security issue you found on Firefox 64 on Ubuntu, is there an issue reported somewhere for that? I'm considering adding a link to it in the security notice.

ylesaout · 2019-01-24T23:09:18Z

You're welcome !
Regarding Firefox, I was not able to find any issue on the Mozilla tracker. Thus I dug a little bit more and filled an issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1522702. Let's see what will happen.

Aaronius · 2019-01-25T00:14:21Z

Thanks. Let me know if you hear back.

Aaronius · 2019-01-25T00:14:24Z

Published in v3.1.0.

ylesaout force-pushed the ylesaout-iframe-id branch from 345189d to d47e298 Compare January 15, 2019 17:36

ylesaout force-pushed the ylesaout-iframe-id branch from d47e298 to 9b6faac Compare January 18, 2019 17:07

ylesaout force-pushed the ylesaout-iframe-id branch from 9b6faac to b7c036f Compare January 21, 2019 17:15

Aaronius requested changes Jan 22, 2019

View reviewed changes

ylesaout force-pushed the ylesaout-iframe-id branch 2 times, most recently from 2860759 to 97e8179 Compare January 22, 2019 22:22

ylesaout force-pushed the ylesaout-iframe-id branch from 2bfdde9 to eb615b6 Compare January 23, 2019 21:11

Le Saout and others added 2 commits January 23, 2019 22:26

Added iframe optional parameter to connectToChild

d12deed

updated the documentation for the iframe option

1857c3b

ylesaout force-pushed the ylesaout-iframe-id branch from eb615b6 to 1857c3b Compare January 23, 2019 21:27

Aaronius merged commit 513d6c0 into Aaronius:master Jan 24, 2019

ylesaout deleted the ylesaout-iframe-id branch January 24, 2019 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the possibility to set an id to the iframe #25

Added the possibility to set an id to the iframe #25

ylesaout commented Jan 15, 2019

Aaronius commented Jan 15, 2019

ylesaout commented Jan 15, 2019

ylesaout commented Jan 16, 2019

Aaronius commented Jan 17, 2019

ylesaout commented Jan 17, 2019

Aaronius commented Jan 18, 2019 •

edited

Loading

ylesaout commented Jan 18, 2019

Aaronius commented Jan 18, 2019 •

edited

Loading

ylesaout commented Jan 18, 2019

Aaronius commented Jan 18, 2019

ylesaout commented Jan 19, 2019

Aaronius commented Jan 19, 2019

ylesaout commented Jan 21, 2019

Aaronius left a comment

Aaronius Jan 22, 2019

ylesaout Jan 22, 2019

Aaronius Jan 23, 2019

ylesaout Jan 23, 2019 •

edited

Loading

Aaronius Jan 22, 2019

ylesaout Jan 22, 2019

Aaronius Jan 22, 2019

ylesaout Jan 22, 2019

Aaronius commented Jan 23, 2019

ylesaout commented Jan 23, 2019

Aaronius commented Jan 24, 2019

ylesaout commented Jan 24, 2019 •

edited

Loading

Aaronius commented Jan 25, 2019

Aaronius commented Jan 25, 2019

Added the possibility to set an id to the iframe #25

Added the possibility to set an id to the iframe #25

Conversation

ylesaout commented Jan 15, 2019

Aaronius commented Jan 15, 2019

ylesaout commented Jan 15, 2019

ylesaout commented Jan 16, 2019

Aaronius commented Jan 17, 2019

ylesaout commented Jan 17, 2019

Aaronius commented Jan 18, 2019 • edited Loading

ylesaout commented Jan 18, 2019

Aaronius commented Jan 18, 2019 • edited Loading

ylesaout commented Jan 18, 2019

Aaronius commented Jan 18, 2019

ylesaout commented Jan 19, 2019

Aaronius commented Jan 19, 2019

ylesaout commented Jan 21, 2019

Aaronius left a comment

Choose a reason for hiding this comment

Aaronius Jan 22, 2019

Choose a reason for hiding this comment

ylesaout Jan 22, 2019

Choose a reason for hiding this comment

Aaronius Jan 23, 2019

Choose a reason for hiding this comment

ylesaout Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Aaronius Jan 22, 2019

Choose a reason for hiding this comment

ylesaout Jan 22, 2019

Choose a reason for hiding this comment

Aaronius Jan 22, 2019

Choose a reason for hiding this comment

ylesaout Jan 22, 2019

Choose a reason for hiding this comment

Aaronius commented Jan 23, 2019

ylesaout commented Jan 23, 2019

Aaronius commented Jan 24, 2019

ylesaout commented Jan 24, 2019 • edited Loading

Aaronius commented Jan 25, 2019

Aaronius commented Jan 25, 2019

Aaronius commented Jan 18, 2019 •

edited

Loading

Aaronius commented Jan 18, 2019 •

edited

Loading

ylesaout Jan 23, 2019 •

edited

Loading

ylesaout commented Jan 24, 2019 •

edited

Loading