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

I have forked this library, let's discuss if we should join efforts or not #211

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

Comments

@franleplant
Copy link

What Is the issue?

Hi guys! Thank you so much for this library it is awesome.

Unfortunately I found several problems while using it that prompted me to fork it and do major
overhaul of the code and I wanted to reach out to you so that I can explain why I did the things I did
and maybe discuss if it is worth it to plan a potential merger down the road, Im super open to that.

Find the fork here https://github.com/franleplant/ibridge I already published as ibridge in npm.
Also, I hope I did the right steps to give you guys correct attributions, if you find something wrong please let me know and I will fix it promptly.

Problems and solutions I found with postmate that lead me to build ibridge

  • .get lacks proper return type support.

The idea of .get is really good but the implementation was not ideal. .get is a way of calling model functions
in the child and retrieving values, but what happens if the model throws an error? this becomes even more important when dealing with promises, let's say fetching stuff, having to treat the errors thrown in the child functions specially so that they can fit the "always resolving" promise paradigm in .get was awkward and problematic.

This makes .get a stub of what it really can be.
With simple tweaks we can make it reject in the parent if it was rejected in the child.

Additionally I removed the capability of model functions to be attributes, for me it does not make sense. Im open to other ideas though.

Related issue #94

  • call is just a weird way of sending events

Intuitively when going through the lib's docs it might appear that call is a way of remotely calling model functions in the child and getting the return values, but it is not at all like that. Most of the things can be resolved with the improved .get I described above.

If you want to call a function in the child without caring about the return values then you still can use .get

BUT I do see the value of allowing freely emitting of events between parent and child and that is why I added emitToChild and emitToParent and on so that if the lib consumer wants to do something outside the main happy path which is get then they do not need to revert to postMessage.

BONUS: I make both child and parent be event emitters for a better composition of event.

  • model[modelKey] is just to basic

Using this lib I found myself wanting to be able to use lodash paths instead of model keys when using .get,
this allows for a better structuring of more complex models. So I added support for parent.get('some.deep.lodash.path').

  • typescript

I am a big fan of typescript and I avoid touching pure javascript, so I migrated it entirely to typescript and it is awesome,
we get some cool capabilities of making the child be generic over the model type.

  • model context

Another thing I really found myself patching around was the lack of model context. So I added the capability of
setting up a context in the child that will be passed as this to all the model functions. We use that internally
to store references to apis such as socketio. Another apis for these sort of thing might be better, Im open to ideas.

  • code structure: a single native event listener, the dispatcher

On postmate there are a bunch of native event listeners setup on and off.
I found that I can get away with a single one (called the dispatcher) that will perform some
verification on the event and then emit internally a higher level event (using an Event Emitter library) which
abstract away a lot of the complexity of verifying, parsing, and the meta structure of the payload that is unwrapped
and used in the higher level events (such as get/resolve get/reject, etc).

This has worked really well, abstracting a lot of complexity from the rest of logic in the class

  • code structure: init classes are out

I didn't like much this intermediate classes that are used to init the real classes, plus returning a promise from class constructors is kind of problem, additionally, by moving this logic inside the main classes we can

  1. reuse the main dispatcher and operate on higher levels (check how nice the handshake logic got thanks to this)
  2. be able to call again the handhsake if needed (sometimes the child page might reload or something).

I did that, but I believe is easy to go back on that and I am completely open to that.

  • some other code changes.

I think that the end result is really nice, it has docs, it has types, I am already using it at work.

I am open to plan a merger, or I am also open to contribute some parts of that lib into this one and keep them separate the.

Thanks!
Fran

@aiji42
Copy link

aiji42 commented Dec 1, 2020

Thank you.
I fully support your efforts. Of course I have respect for the authors and contributors of postmate.

@alesgenova
Copy link

Funny, pretty much on the same day you started your fork I also started a rewrite after getting frustrated with some of the issues you mention.

I also opted for TypeScript, and added support for communicating with webworkers instead of iframes only, among other new features.

https://github.com/alesgenova/post-me

Unfortunately this issue wasn't up at the time I was working on it (Nov 27th-29th), would have been nice to collaborate

@franleplant
Copy link
Author

hey @alesgenova yours is a really interesting one, maybe we can make our libs a single one? It's interesting to think that we can conceive it as a wrapper on top of postMessage that can bidirectionally expose methods (aka models) on either side.

@alesgenova
Copy link

@franleplant Yeah I think it could be a good to merge the ideas we had in our respective implementations into a single better library. Maybe let's have a chat about it in the next few days. My email is my github username at gmail.

@HyperCharlie
Copy link

I am looking at rewriting this library in typescript as well, and ibridge is pretty close to what I had planned. My only objection and reason to avoid using it is your removal of the .call() method. Yes, underneath it is doing events, but wrapping it up to look and work like a promise-based RPC is just a nice thing to have! I suppose my love for it is due to putting a layer around Postmate with my own classes that turns it into an actual RPC mechanism, so perhaps instead of ditching it entirely it would be better to more fully develop the concept?

@franleplant
Copy link
Author

franleplant commented Mar 11, 2021 via email

@mathpaquette
Copy link

What Is the issue?

Hi guys! Thank you so much for this library it is awesome.

Unfortunately I found several problems while using it that prompted me to fork it and do major overhaul of the code and I wanted to reach out to you so that I can explain why I did the things I did and maybe discuss if it is worth it to plan a potential merger down the road, Im super open to that.

Find the fork here https://github.com/franleplant/ibridge I already published as ibridge in npm. Also, I hope I did the right steps to give you guys correct attributions, if you find something wrong please let me know and I will fix it promptly.

Problems and solutions I found with postmate that lead me to build ibridge

  • .get lacks proper return type support.

The idea of .get is really good but the implementation was not ideal. .get is a way of calling model functions in the child and retrieving values, but what happens if the model throws an error? this becomes even more important when dealing with promises, let's say fetching stuff, having to treat the errors thrown in the child functions specially so that they can fit the "always resolving" promise paradigm in .get was awkward and problematic.

This makes .get a stub of what it really can be. With simple tweaks we can make it reject in the parent if it was rejected in the child.

Additionally I removed the capability of model functions to be attributes, for me it does not make sense. Im open to other ideas though.

Related issue #94

  • call is just a weird way of sending events

Intuitively when going through the lib's docs it might appear that call is a way of remotely calling model functions in the child and getting the return values, but it is not at all like that. Most of the things can be resolved with the improved .get I described above.

If you want to call a function in the child without caring about the return values then you still can use .get

BUT I do see the value of allowing freely emitting of events between parent and child and that is why I added emitToChild and emitToParent and on so that if the lib consumer wants to do something outside the main happy path which is get then they do not need to revert to postMessage.

BONUS: I make both child and parent be event emitters for a better composition of event.

  • model[modelKey] is just to basic

Using this lib I found myself wanting to be able to use lodash paths instead of model keys when using .get, this allows for a better structuring of more complex models. So I added support for parent.get('some.deep.lodash.path').

  • typescript

I am a big fan of typescript and I avoid touching pure javascript, so I migrated it entirely to typescript and it is awesome, we get some cool capabilities of making the child be generic over the model type.

  • model context

Another thing I really found myself patching around was the lack of model context. So I added the capability of setting up a context in the child that will be passed as this to all the model functions. We use that internally to store references to apis such as socketio. Another apis for these sort of thing might be better, Im open to ideas.

  • code structure: a single native event listener, the dispatcher

On postmate there are a bunch of native event listeners setup on and off. I found that I can get away with a single one (called the dispatcher) that will perform some verification on the event and then emit internally a higher level event (using an Event Emitter library) which abstract away a lot of the complexity of verifying, parsing, and the meta structure of the payload that is unwrapped and used in the higher level events (such as get/resolve get/reject, etc).

This has worked really well, abstracting a lot of complexity from the rest of logic in the class

  • code structure: init classes are out

I didn't like much this intermediate classes that are used to init the real classes, plus returning a promise from class constructors is kind of problem, additionally, by moving this logic inside the main classes we can

  1. reuse the main dispatcher and operate on higher levels (check how nice the handshake logic got thanks to this)
  2. be able to call again the handhsake if needed (sometimes the child page might reload or something).

I did that, but I believe is easy to go back on that and I am completely open to that.

  • some other code changes.

I think that the end result is really nice, it has docs, it has types, I am already using it at work.

I am open to plan a merger, or I am also open to contribute some parts of that lib into this one and keep them separate the.

Thanks! Fran

why not collaborating to this one instead ? did you ever try to create a pull request here ?

@mathpaquette
Copy link

guys, found another pretty interesting implementation and still very active. https://github.com/Aaronius/penpal

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

5 participants