franleplant / ibridge

Typesafe iframe bridge for easy parent child bidirectional communication
https://www.npmjs.com/package/ibridge
36 stars 3 forks source link

v1 and post-me merge discussion #10

Open franleplant opened 3 years ago

franleplant commented 3 years ago

Refs

Objectives

Step 0: browser level automatic integration testing

Before anything else I want to setup a proper browser level integration testing for ibridge (and potentially the resulting merge). Right now I am using jsdoc with a bunch of mocks which is a similar approach to what post-me is doing although they are doing it arguably better and so the library does have automatic testing coverage but I find that the real test is always open the examples manually and checking that the thing really works because that way I can observe the inner workings of browsers and how they behave regarding origins and weird stuff that may happen at the postMessage border.

This will give us the confidence to iterate more quickly and to add more test cases when adding more features to this lib, unfortunately jsdom has some limitations in its implementation and for all intents and purposes is not a real browser.

ref

What do we need?

I have experience with selenium and it would be more than enough but I haven't had the chance to set it up properly, that's probably going to be my first line of investigation.

Step 1: the merge

I would like to focus on the things I like the most from both libraries

ibridge

Post-me

Comments out of the above

So for example, today in ibridge.Parent we have

https://github.com/franleplant/ibridge/blob/f1be11df884f76f2106a5445556b59b45acf2926/src/Parent.ts#L38-L68

and in the handshake we have

https://github.com/franleplant/ibridge/blob/8b13eb1dfd3b3c2692b091600df0daa04b33f658/src/Parent.ts#L132-L133

This could probably be abstracted away with a promise passed as a parameter


function getRemoteWindow(container: HtmlElement, url: string): Promise<Window> {
  return new Promise((resolve) => {
    const iframe = document.createElement("iframe");
    container.appendChild(iframe);
    iframe.src = url;

    iframe.onLoad(() => resolve(iframe.contentWindow))
  })
}

// usage

const iparent  = new ibridge.Parent({getRemoteWindow: () => getRemoteWindow(someContainer, someUrl)})

// or even

const remoteWindow = await getRemoteWindow(someContainer, someUrl)
const remoteOrigin = remoteWindow.origin
const iparent = new ibridge.Parent({remoteWindow, remoteOrigin})

This enables us to remove the logic of setting up the iframe, and hooking the onLoad event at the right moment (I had problems with this in the past) and simply changing it for a promise that resolves when the window is abailable and loaded, no need to worry about anything else really. We could provide the getRemoteWindow as a higher level api like we do today but this enables us to easily let users do whatever they want with the window creating plus it can be useful when also supporting webworkers etc.

Style

Closing

There's so much work to do Im getting excited, really good ideas on the table!

cc @alesgenova

alesgenova commented 3 years ago

Hey thanks for putting this document together, lots of good ideas. Gonna put some comments below. Btw, I'm gonna have a fair amount of time in the next week or so, if you do too I think we can get a lot done. How should we coordinate on the work to be done?

Refs

* [postmate original discussion](https://github.com/dollarshaveclub/postmate/issues/211#issuecomment-750493853)

* [post-me](https://github.com/alesgenova/post-me)

Objectives

* see if we can merge post-me and ibridge to consolidate efforts

* if the merge is not possible I would like to take the better ideas from post-me and use them on ibridge

* solve some of the issues users have opened on `ibridge` holistically

Step 0: browser level automatic integration testing

Before anything else I want to setup a proper browser level integration testing for ibridge (and potentially the resulting merge). Right now I am using jsdoc with a bunch of mocks which is a similar approach to what post-me is doing although they are doing it arguably better and so the library does have automatic testing coverage but I find that the real test is always open the examples manually and checking that the thing really works because that way I can observe the inner workings of browsers and how they behave regarding origins and weird stuff that may happen at the postMessage border.

100% agree, when I implemented the tests for post-me I had to work around several limitations from the mocks privided by jsdoc, to the point that I couldn't even have a proper test for the worker case (not for lack of trying).

This will give us the confidence to iterate more quickly and to add more test cases when adding more features to this lib, unfortunately jsdom has some limitations in its implementation and for all intents and purposes is not a real browser.

ref

What do we need?

* headless browser testing, the tool really does not matter, can be selenium, browser stack, etc

* it must be easily compatible with github actions

* it should be pretty straight forward to install (i.e. `npm install` is all that's needed)

I have experience with selenium and it would be more than enough but I haven't had the chance to set it up properly, that's probably going to be my first line of investigation.

Step 1: the merge

I would like to focus on the things I like the most from both libraries

ibridge

* I love the name

Sure, I have no strong feelings about the post-me name at all, I also think ibridge is better. In terms of of where the code lives, I think we could create an ibridge organization of which we would both be members, and have the library repo be there. Thoughts?

* we have a logo

* visual docs and a good amount of docs

* I like the idea of having a single event listener `the dispatcher` and then delegating the event handling to `Emittery`, this allows us to express complex event driven logic in a very natural way i.e. [the handshake](https://github.com/franleplant/ibridge/blob/8b13eb1dfd3b3c2692b091600df0daa04b33f658/src/Parent.ts#L98-L118) and also allow users to build more complex event driven flows using higher level event emitter apis that I believe are far superior that the native ones.

* I like the idea of making the `handshake` a simple method instead of a separate thing like postmate and post-me do because it allows us to reuse the main dispatcher and all the high level goodies and infrastructure but also it enables us and users to recall the handshake if needed.

I'm not sure I'm following the two points above (I'm not familiar with emittery, will look into it), but yeah I think that the handshake code was the weakest part of my post-me library, so I'm open to

* I like of course `emitToParent` and `emitToChild` methods (see the post-me comments below).

Yeah, I think that calling/emitting on both ends is a must. From the api point of view I think I like it better how it's done in post-me with the local/remote handle, andI think you're also saying the same below

  • I like how get is structured (which is pretty similar to what post-me does)

  • I like how get accepts an object path to look for the model function, this allows users to build more complex models and structure them more naturally

  • I like the idea of model context, perhaps the implementation is not ideal but I do believe that solving that issue is important

I didn't quite understand the need for the context to be honest. It seemed to me that it's something that the user of the library could solve on their own via closure or other means. But if you have strong feelings about it we can keep it around (optionally?).

* i love the idea of having a single wrapper postMessage event that internally uses a higher level structure that can be easily mapped into Emittery (or any other even emitter really) events.

Post-me

* webworker support

* bidirectional `get` support (although post-me calls it `call` which is probably a better name)

* the concept of `localHandle` and `remoteHandle`, these are amazing plus I love `local` and `remote` as words to define the real concepts, it is much better than parent and simply `emit`.

* it doesn't try to create the iframe like ibridge and postmate do, instead it accepts a window and lets consumer handle that (is not that hard after all)

* it accepts the origin as an argument instead of trying to be smart about it and detect it. I don't believe that restricting origins really adds and strong security layer to the library or to the `postMessage` (im open to being proven wrong) and I believe that the real security happens at the HTTP headers such as `content-policy: frame-ancestors ...`, so I **really like not having to worry to much about this inside the library**. If the consumers want to add the extra layer of security with a custom `origin` then let them handle that by themselves. There are some considerations to have here https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#Syntax , maybe we can enable the `autodetect` mode in the child and in the parent, something like what we have today in ibridge

As I mentioned above, the handshake api is the part I was the least happy with, so rethinking it from the ground can only make it better. The reason I was passing the origin to the handshake was to forward it to the postMessage call, and to make a check in onmessage, as recommended by the same MDN page you linked. You may be right that the header could remove the need for this, just want to make sure before we introduce a security hole!

Comments out of the above

* i looks like the v1 solution can be structured as a

  * low level `w.postMessage` and `w.addEventListener('message')` abstraction that is generic on `w` (i.e. window, worker, etc) that does not care how that `window` is created, be it the main window of the parent document or the window of an iframe or the global object of a web worker.
  * a handshake mechanism between parent and child, local and remote, messenger and messenger, server and client, etc
  * a higher level api on top that abstracts common stuff such as hidden iframe creation, web worker spawning etc.

So for example, today in ibridge.Parent we have

https://github.com/franleplant/ibridge/blob/f1be11df884f76f2106a5445556b59b45acf2926/src/Parent.ts#L38-L68

and in the handshake we have

https://github.com/franleplant/ibridge/blob/8b13eb1dfd3b3c2692b091600df0daa04b33f658/src/Parent.ts#L132-L133

This could probably be abstracted away with a promise passed as a parameter

function getRemoteWindow(container: HtmlElement, url: string): Promise<Window> {
  return new Promise((resolve) => {
    const iframe = document.createElement("iframe");
    container.appendChild(iframe);
    iframe.src = url;

    iframe.onLoad(() => resolve(iframe.contentWindow))
  })
}

// usage

const iparent  = new ibridge.Parent({getRemoteWindow: () => getRemoteWindow(someContainer, someUrl)})

// or even

const remoteWindow = await getRemoteWindow(someContainer, someUrl)
const remoteOrigin = remoteWindow.origin
const iparent = new ibridge.Parent({remoteWindow, remoteOrigin})

This enables us to remove the logic of setting up the iframe, and hooking the onLoad event at the right moment (I had problems with this in the past) and simply changing it for a promise that resolves when the window is abailable and loaded, no need to worry about anything else really. We could provide the getRemoteWindow as a higher level api like we do today but this enables us to easily let users do whatever they want with the window creating plus it can be useful when also supporting webworkers etc.

Style

* post-me has a style that I'm not super fond of: top level functions use arrow functions and in some cases it gets overly complicated, my motto is [`clarity then simplicity...`](https://nosleepjavascript.com/programming-principles/) and I'd like to maximize that

Closing

There's so much work to do Im getting excited, really good ideas on the table!

Yes it is exciting! Would you like to have a call to iron out anything remaining and start making a plan for the work to be done?

cc @alesgenova

franleplant commented 3 years ago

Hi!

I think we could create an ibridge organization of which we would both be members, and have the library repo be there. Thoughts?

yes!

I didn't quite understand the need for the context to be honest. It seemed to me that it's something that the user of the library could solve on their own via closure or other means. But if you have strong feelings about it we can keep it around (optionally?).

I am still on the fence, it kinda makes sense to allow consumers to bind the model/method functions to something potentially useful but on the other hand I agree that there are many other ways of doing this at the consumer side, I really don't know so whatever you feel is better Im probably fine with.

As I mentioned above, the handshake api is the part I was the least happy with, so rethinking it from the ground can only make it better. The reason I was passing the origin to the handshake was to forward it to the postMessage call, and to make a check in onmessage, as recommended by the same MDN page you linked. You may be right that the header could remove the need for this, just want to make sure before we introduce a security hole!

I think you are missing my point: I like what post-me does and we should probably just copy it.

Let's schedule a call next week, send me an email at franleplant@gmail.com , I am at UTC-3 so if you are in the American continent it will be pretty easy for us to get together in terms of hours. I will be partially working on my paid job next week but I will get some days off, so after 4 or 5 PST I am probably all good.

PS

I had a freaking obsession spike with all the things we've discussed and I took the liberty to do some very rough draft of the new arch incorporating a lot of the things we've discussed before, it is not definitive by no meas but it might be a good start, perhaps you can take a look and even play around with it. https://github.com/franleplant/ibridge/pull/11

More comments over there

cmawhorter commented 3 years ago

I've migrated an app from using postmate to ibridge, and now to post-me so hopefully these thoughts are helpful.

Why rewrite ibridge when it seems like it could switch to using post-me under-the-hood. There is some overlap, but the two libraries don't exactly have the same goals. post-me feels low-level whereas ibridge doesn't. ibridge is almost a drop-in replacement for postmate, post-me isn't.

By leaning on post-me for the lowlevel stuff, that frees up ibridge to handle high-level stuff like auto reconnect or maybe supporting websockets and allowing an app to span multiple devices.

franleplant commented 3 years ago

Hi Cory! Thanks for reaching out, we definitely hear you and agree with you, the idea is to unify both libraries but still provide the same functionality and more while also unifying efforts and community. You will still be able to do all the stuff you are currently doing but with a single lib! We have exciting plans and you are more than welcomed to participate. Also you can request features by opening issues. All new progress will be made in https://github.com/ibridge-js/ibridge

On Tue, 29 Dec 2020 at 01:05 Cory Mawhorter notifications@github.com wrote:

I've migrated an app from using postmate to ibridge, and now to post-me so hopefully these thoughts are helpful.

Why rewrite ibridge when it seems like it could switch to using post-me under-the-hood. There is some overlap, but the two libraries don't exactly have the same goals. post-me feels low-level whereas ibridge doesn't. ibridge is almost a drop-in replacement for postmate, post-me isn't.

By leaning on post-me for the lowlevel stuff, that frees up ibridge to handle high-level stuff like auto reconnect https://github.com/dollarshaveclub/postmate/issues/126 or maybe supporting websockets and allowing an app to span multiple devices.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/franleplant/ibridge/issues/10#issuecomment-751938875, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOEIJ2ZEBEIFEBTWXR4MKDSXFIPZANCNFSM4VJI355Q .

miguelperez commented 3 years ago

Hi, thanks for this. I am about to start working on a project and I would like to know which library should I start using now that will allow me to migrate/update in the future to the new library once the new version is up?

alesgenova commented 3 years ago

Hi, thanks for this. I am about to start working on a project and I would like to know which library should I start using now that will allow me to migrate/update in the future to the new library once the new version is up?

The merge experiment didn't work out in the end, so the two libraries will continue independently.

I guess it's up to you to use the whichever has the features/api you like better :)

franleplant commented 3 years ago

Hi Miguel! Yes, the merge end up not happening so ibridge will remain ibridge, there is a open wip PR with some of the upcoming changes, the API will remain generally the same but will be more flexible and some breaking changes (same functionality slightly different api) will be documented

On Wed, 13 Jan 2021 at 09:49 Alessandro Genova notifications@github.com wrote:

Hi, thanks for this. I am about to start working on a project and I would like to know which library should I start using now that will allow me to migrate/update in the future to the new library once the new version is up?

The merge experiment didn't work out in the end, so the two libraries will continue independently.

I guess it's up to you to use the whichever has the features/api you like better :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/franleplant/ibridge/issues/10#issuecomment-759427277, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOEIJ4ECJVZSBUUV7XLCQTSZWJDZANCNFSM4VJI355Q .

miguelperez commented 3 years ago

Thanks both of you for the hard work!