postmate
postmate copied to clipboard
Made communication identical in both directions. Removed modeling. Removed classes.
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:
- 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.
- I believe communication in both directions (parent-to-child, child-to-parent) should be identical and support passing data and returning a value.
- I don't believe classes provide value for what's trying to be accomplished. It's slightly awkward to me that
new
ing 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.
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!
Due to the lack of response, I've assumed there isn't interest in these changes. Closing.
@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 ;)
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. :/
Will discuss more soon!
- 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')
});
- If multiple instances of Postmate are running on the same page, does removing the constructor cause communication issues?
- Is there a need to use
setPromise
andsetDebug
as opposed to setting properties directly?
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.
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.
- I could see Postmate handling identical communication on both the parent and child
- 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.
- The function names
connectParent
andconnectChild
might change
What are your thoughts?
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.
@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 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 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 Yes, definitely! I will reach out to you soon.
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?
@Andarist @brucou I’m going to revisit this very soon for a series of updates to Postmate, thanks for all of your patience!