Added support for promises #4
Conversation
|
Hi @israelss , |
|
This should definatelly be merged! |
|
Hello! First of all, I am so truly sorry for the long hiatus on this project! @BluuArc thank you for your contribution!! Can you include some documentation, complementing the README regarding the possibilities to use Promises? |
Hi @mgesmundo! |
Glad to hear that you're back! I'm usually busy during the week days, but I'll see if I can get to this by the end of the week. |
README.md
Outdated
| ### **1.2.1** | ||
| * Added support for promises as input functions | ||
| * TODO: more extensive testing needed, but all 34 integration tests and 242 unit tests pass at time of writing | ||
|
|
||
| ### **1.2.0** | ||
|
|
||
| #### _Highlights:_ |
There was a problem hiding this comment.
Please add some reference/examples on how to use it with Promises on API section.
There was a problem hiding this comment.
Added some examples in my recent commits.
Thank you for your patience and contribution! |
… with browsers that do not support arrow functions; minor cleanup of worker function to reduce duplication
…error message when converting a circular structure to JSON
…uce new build with promise support
| @@ -1,5 +1,7 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased | |||
There was a problem hiding this comment.
I moved my previous changes in the README for changelog updates to the changelog file under a new Unreleased header since I'm not sure under what version you'd release this under, and it didn't feel right being presumptuous about the version being made in the README since I'm not the one producing a new package.
There was a problem hiding this comment.
I'll release under v1.2.1, you were right before 😄
But I do understand why you did change. I'll ask you to maintain like that, because I do intend to make some other changes, and I'll change it back to 1.2.1 when it's time to merge at master. Thank you!
| @@ -51,7 +51,7 @@ const argumentError = ({ expected = '', received, extraInfo = '' }) => { | |||
| try { | |||
| return new TypeError(`${'You should provide ' + expected}${'\n' + extraInfo}${'\nReceived: ' + JSON.stringify(received)}`) | |||
| } catch (err) { | |||
| if (err.message === 'Converting circular structure to JSON') { | |||
| if (err.message.indexOf('Converting circular structure to JSON') > -1) { | |||
There was a problem hiding this comment.
I updated this line to fix a failing unit test and because apparently on newer browsers the error message itself includes more details on the property having a circular dependency.
There was a problem hiding this comment.
I noticed that also. Thank you for the update!
|
@BluuArc between work and studies I am very busy in the last couple of weeks. I will do my best to review your changes this weekend. Also, I'm thinking of include your work into another branch, and later merge it with master. This weekend I will let you know. Thank you again for your contribution and patience! |
| @@ -1,5 +1,7 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased | |||
There was a problem hiding this comment.
I'll release under v1.2.1, you were right before 😄
But I do understand why you did change. I'll ask you to maintain like that, because I do intend to make some other changes, and I'll change it back to 1.2.1 when it's time to merge at master. Thank you!
| @@ -51,7 +51,7 @@ const argumentError = ({ expected = '', received, extraInfo = '' }) => { | |||
| try { | |||
| return new TypeError(`${'You should provide ' + expected}${'\n' + extraInfo}${'\nReceived: ' + JSON.stringify(received)}`) | |||
| } catch (err) { | |||
| if (err.message === 'Converting circular structure to JSON') { | |||
| if (err.message.indexOf('Converting circular structure to JSON') > -1) { | |||
There was a problem hiding this comment.
I noticed that also. Thank you for the update!
Added support for promises in input functions. Even though all 34 integration tests and 242 unit tests pass at time of writing, I think more extensive testing is needed. Should theoretically fix the Promise cloning issue in #3 .