postmate icon indicating copy to clipboard operation
postmate copied to clipboard

Made communication identical in both directions. Removed modeling. Removed classes.

Open Aaronius opened this issue 8 years ago • 15 comments

Hello Dollar Shave Club! First of all, thanks for all the work you've put into this project. Your use of promises and closures is really smart. I have some feedback that I hope you'll consider. I won't be offended if you disagree or reject the PR outright. If you end up wanting to merge this PR, I'd be happy to update the docs (both the readme and code docs). Here are my thoughts:

  1. I believe mixing concerns of communication and modeling isn't a Good Thingâ„¢. I believe it would be better if Postmate were limited to just communication. If a consumer wants to back the communication with some sort of modeling then they should still be able to do so.
  2. I believe communication in both directions (parent-to-child, child-to-parent) should be identical and support passing data and returning a value.
  3. I don't believe classes provide value for what's trying to be accomplished. It's slightly awkward to me that newing the Postmate class results in a promise of a handshake negotiation. Classes also add some weight in babel overhead.

In this PR I've provided an alternative API that addresses the above points.

Here's how consumption is intended to work:

Parent:

import { connectParent } from 'postmate';

connectParent({
  container: document.getElementById('frame'),
  url: 'http://localhost:9000/child.html',
  methods: {
    // Methods exposed to the child.
    add(num1, num2) { return num1 + num2; }
  }
}).then(child => {
  child.multiply(2, 3).then(total => console.log(total));
});

Child:

import { connectChild } from 'postmate';

connectChild({
  methods: {
    // Methods exposed to the parent.
    multiply(num1, num2) { return num1 * num2; }
  }
}).then(parent => {
  parent.add(6, 2).then(total => console.log(total));
});

If a method must do something asynchronously before returning a value, the method may return a promise:

Child:

import { connectChild } from 'postmate';

connectChild({
  methods: {
    // Methods exposed to the parent.
    multiply(num1, num2) { return Promise.resolve(num1 * num2); }
  }
}).then(parent => {
  parent.add(6, 2).then(total => console.log(total));
});

With this design, call, on, emit, and get go away.

To change the Promise implementation:

import { setPromise } from 'postmate';

setPromise(RSVP.Promise);

To set debugging:

import { setDebug } from 'postmate';

setDebug(true);

You may notice that I changed the gulp code for building the library. Without the changes, I was getting a bunch of code from babel that wasn't providing value.

This PR drops the size of the lib by a KB.

Aaronius avatar Oct 23 '16 20:10 Aaronius

Hi there. Just checking in to see if you may have had time to look this over. My team could use (need) these changes pretty soon. Thanks!

Aaronius avatar Oct 25 '16 21:10 Aaronius

Due to the lack of response, I've assumed there isn't interest in these changes. Closing.

Aaronius avatar Oct 29 '16 21:10 Aaronius

@Aaronius Hi! Didn't notice this PR until now! Let me review it and we will have an open discussion regarding this. Been very busy this week ;)

jakiestfu avatar Oct 29 '16 21:10 jakiestfu

Sorry, my team needed them quickly and since the entire API was changed I figured there wasn't much interest in trying to merge the changes. I polished them up, changed several other things, and released it as PenPal. This is awkward. :/

Aaronius avatar Oct 29 '16 21:10 Aaronius

Will discuss more soon!

jakiestfu avatar Oct 29 '16 21:10 jakiestfu

  1. What happens when I want to share properties that aren't methods? I.e. primitives or Promises directly?
new Postmate.Model({
  foo: 'bar',
  user: fetch('/user.json')
});
  1. If multiple instances of Postmate are running on the same page, does removing the constructor cause communication issues?
  2. Is there a need to use setPromise and setDebug as opposed to setting properties directly?

jakiestfu avatar Oct 29 '16 23:10 jakiestfu

Regarding (1) you can expose a generic get method from the child if you would like:

Child

import { connectChild } from 'postmate';

const myModel = {
  foo: 'bar',
  user: fetch('/user.json')
};

connectChild({
  methods: {
    // Methods exposed to the parent.
    get(propName) { return myModel[propName];  }
  }
});

Parent

import { connectParent } from 'postmate';

connectParent({
  url: 'http://localhost:9000/child.html'
}).then(child => {
  child.get('foo').then(value => console.log(value));
  child.get('user').then(value => console.log(value));
});

Regarding (2), no, using a constructor doesn't provide any particular benefit here.

Regarding (3), in this PR I chose to use named exports instead of a default export. This is an es6 module thing (I'm not sure how familiar you are with es6 modules; this can help if you're not). If you import a primitive named export (e.g., import { debug } from 'penpal';) and then set it to a different value (e.g., debug = true) , the change doesn't "make it's way up" to the module the primitive came from (due to primitive variables being value-based and not reference-based). Anyway, long story short, in the end I decided to use a default export and you can set Promise and debug as you describe. I've made a number of changes since submitting this PR.

Aaronius avatar Nov 01 '16 03:11 Aaronius

Thanks for the further explanation! There is a world in which I see merging these changes, but I have some personal issues with the differences between the two.

  1. I could see Postmate handling identical communication on both the parent and child
  2. We like the simplicity of exposing a model on each side as opposed to merely a set of methods. It makes it more difficult to implement things that postmate could easily handle. It's sole purpose is not to handle promises, rather, many types of data including promises.
  3. The function names connectParent and connectChild might change

What are your thoughts?

jakiestfu avatar Nov 01 '16 03:11 jakiestfu

Regarding:

It makes it more difficult to implement things that postmate could easily handle. It's sole purpose is not to handle promises, rather, many types of data including promises.

I'd like to know more about what these things are. The way I've set it up, the get function in my previous example can also return many types of data including promises.

Regarding the name change, I've already changed them to connectToChild and connectToParent in PenPal.

Aaronius avatar Nov 01 '16 04:11 Aaronius

@yowainwright @jakiestfu is there anything I could do to push this forward? Im interested in remote calls from child to parent, can dedicate to working on this.

Andarist avatar Feb 24 '18 16:02 Andarist

@Andarist yes!!! 🎉

We're planning on it. I've been working on updates (as you're aware of) to hopefully prevent regressions when we make the switch. I will try to get testing updates done ASAP for ya'!

yowainwright avatar Feb 27 '18 04:02 yowainwright

@yowainwright if you need any help with the implementation or discussing the API (not sure if you want to release new version or do you want to avoid breaking changes) I would be happy to help/discuss :)

Andarist avatar Feb 27 '18 08:02 Andarist

@Andarist Yes, definitely! I will reach out to you soon.

yowainwright avatar Feb 27 '18 16:02 yowainwright

what is the status of that? Is it better to directly use penpal instead, with the anticipation that this will not be merged anytime soon?

brucou avatar Dec 11 '18 22:12 brucou

@Andarist @brucou I’m going to revisit this very soon for a series of updates to Postmate, thanks for all of your patience!

jakiestfu avatar Dec 12 '18 17:12 jakiestfu