Added the possibility to set an id to the iframe

ylesaout commented 5 years ago

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

Aaronius commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/Aaronius/penpal/pull/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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/dollarshaveclub/postmate/pull/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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ylesaout commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Thanks. Let me know if you hear back.

Aaronius commented 5 years ago

Published in v3.1.0.

Aaronius / penpal

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