Skip to content

Allow execute to return Promise<Value> or Value#65

Merged
andrerpena merged 1 commit intonubasedev:masterfrom
hygraph:master
Dec 7, 2017
Merged

Allow execute to return Promise<Value> or Value#65
andrerpena merged 1 commit intonubasedev:masterfrom
hygraph:master

Conversation

@flexzuu
Copy link
Copy Markdown

@flexzuu flexzuu commented Dec 6, 2017

This allows to use async behavior in the execute function like fetching urls from a server.

This allows to use async behavior in the execute function like fetching urls from a server.
@andrerpena
Copy link
Copy Markdown
Collaborator

Thanks for your contribution @flexzuu . I assume you tested it and it doesn't break the UI or anything, right? I'll test it tonight too and publish it to NPM.

Thanks again.

@andrerpena andrerpena merged commit b85e3bb into nubasedev:master Dec 7, 2017
@flexzuu
Copy link
Copy Markdown
Author

flexzuu commented Dec 7, 2017

Yeah i use it in my stuff and it did not break anything. But there came up a thought that there might be a environment where Promise is not defined and the check might fail. Maybe you can check this but i will do a little research as well.

@andrerpena
Copy link
Copy Markdown
Collaborator

andrerpena commented Dec 7, 2017

Thanks @flexzuu. I just thought of something else now. I'm not sure but It seems more consistent to me that commands always return a Promise<Value> instead of either returning it or Value. I can just make all the current commands to return Promise.resolve().

What do you think? I'll think about this tonight too.

@flexzuu
Copy link
Copy Markdown
Author

flexzuu commented Dec 8, 2017

yeah would simplify the code to only return Promise but that would mean to introduce a breaking change in the commands api. I dunno it's a tradeof between simple code and breaking the commands api or harder to maintain code and non breaking but complicating the api.

Only thing is that i need a distributed version with the promise support in some days (best monday) but i might aswell upload my fork to npm if you need more time. But please let me know so i can start working out how the release process for a fork would look like.

@andrerpena
Copy link
Copy Markdown
Collaborator

andrerpena commented Dec 8, 2017

Ok @flexzuu. I'll agree. The reason I didn't publish your change yesterday was because I wanted to think a bit and hear from you.

Here's what I'll do: This Saturday (tomorrow) I'll publish a non-breaking 2.* version with your changes in it. But I'll update the readme file stating the commands that don't return Promise<value> will be deprecated in 3.*. To be honest... This breaking change will have zero effect on 99% of the users, that just stick with the default ones.

Let's follow this plan. Expect a new release on NPM tomorrow.

@flexzuu
Copy link
Copy Markdown
Author

flexzuu commented Dec 8, 2017

@andrerpena nice solution you worked out i like it 👍
Thanks for the great markdown editor again.

@andrerpena
Copy link
Copy Markdown
Collaborator

andrerpena commented Dec 8, 2017

Thanks for your kind words and your contribution to this project @flexzuu

@andrerpena
Copy link
Copy Markdown
Collaborator

Published as part of 2.2.0

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

Successfully merging this pull request may close these issues.

2 participants