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

v1 and post-me merge discussion #10

Open
franleplant opened this issue Dec 25, 2020 · 8 comments
Open

v1 and post-me merge discussion #10

franleplant opened this issue Dec 25, 2020 · 8 comments

Comments

@franleplant
Copy link
Owner

Refs

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.

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
  • 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 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 like of course emitToParent and emitToChild methods (see the post-me comments 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 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

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

ibridge/src/Parent.ts

Lines 38 to 68 in f1be11d

constructor({
container = document.body,
url,
name = "",
classList = [],
showIframe = false,
}: IConstructorArgs) {
super();
this.url = url;
this.container = container;
this.parent = window;
this.frame = document.createElement("iframe");
this.frame.name = name;
this.frame.classList.add(...classList);
if (!showIframe) {
// Make it invisible
this.frame.style.width = "0";
this.frame.style.height = "0";
this.frame.style.border = "0";
}
debug("Loading frame %s", url);
this.container.appendChild(this.frame);
this.child =
this.frame.contentWindow ||
(this.frame.contentDocument as any)?.parentWindow;
this.childOrigin = resolveOrigin(url);
debug("setting up main listeners");
this.parent.addEventListener("message", this.dispatcher.bind(this), false);
}

and in the handshake we have

ibridge/src/Parent.ts

Lines 132 to 133 in 8b13eb1

// kick the iframe loading process off
this.frame.src = this.url;

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... and I'd like to maximize that

Closing

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

cc @alesgenova

@alesgenova
Copy link

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

ibridge/src/Parent.ts

Lines 38 to 68 in f1be11d

constructor({
container = document.body,
url,
name = "",
classList = [],
showIframe = false,
}: IConstructorArgs) {
super();
this.url = url;
this.container = container;
this.parent = window;
this.frame = document.createElement("iframe");
this.frame.name = name;
this.frame.classList.add(...classList);
if (!showIframe) {
// Make it invisible
this.frame.style.width = "0";
this.frame.style.height = "0";
this.frame.style.border = "0";
}
debug("Loading frame %s", url);
this.container.appendChild(this.frame);
this.child =
this.frame.contentWindow ||
(this.frame.contentDocument as any)?.parentWindow;
this.childOrigin = resolveOrigin(url);
debug("setting up main listeners");
this.parent.addEventListener("message", this.dispatcher.bind(this), false);
}

and in the handshake we have

ibridge/src/Parent.ts

Lines 132 to 133 in 8b13eb1

// kick the iframe loading process off
this.frame.src = this.url;

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
Copy link
Owner Author

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. #11

More comments over there

@franleplant franleplant mentioned this issue Dec 27, 2020
@cmawhorter
Copy link

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
Copy link
Owner Author

franleplant commented Dec 30, 2020 via email

@miguelperez
Copy link

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
Copy link

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
Copy link
Owner Author

franleplant commented Jan 13, 2021 via email

@miguelperez
Copy link

Thanks both of you for the hard work!

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

No branches or pull requests

4 participants