-
Notifications
You must be signed in to change notification settings - Fork 184
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
Made communication identical in both directions. Removed modeling. Removed classes. #15
base: master
Are you sure you want to change the base?
Conversation
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! |
new Postmate.Model({
foo: 'bar',
user: fetch('/user.json')
});
|
Regarding (1) you can expose a generic 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., |
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.
What are your thoughts? |
Regarding:
I'd like to know more about what these things are. The way I've set it up, the Regarding the name change, I've already changed them to |
@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? |
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:
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:
Child:
If a method must do something asynchronously before returning a value, the method may return a promise:
Child:
With this design,
call
,on
,emit
, andget
go away.To change the Promise implementation:
To set debugging:
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.