Skip to content
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

Port Yaku to core-js #170

Closed
wants to merge 1 commit into from
Closed

Port Yaku to core-js #170

wants to merge 1 commit into from

Conversation

ysmood
Copy link

@ysmood ysmood commented Feb 15, 2016

Continue after the #115. Sorry for so long. This time everything goes well. Only one file changed (92162b4), all the tests should pass.

When I debug the project, most of the unit test time is taken by the promise, it's unbearable for new contributors. Pulling the core code of promise out of the project will make the project more maintainable.

You don't have to run the promises-aplus-tests any more. Because as a separate project, yaku has already maintained the promises-aplus-tests on itself. So that you can focus on other parts of the project. Besides, I tested core-js against the promises-es6-tests, it didn't pass all the tests. Yaku did pass them all.

With Yaku, the performance will be better, the error handling will be less painful (such as long stack trace support), the gzipped size of the core.min.js won't change (27KB).

Here's the comparison:

name unit tests 1ms async task optional helpers helpers min js
yaku@0.11.6 330ms / 106MB 29 3.8KB
bluebird@3.3.1 x (18 failing) 265ms / 88MB partial 100 52.2KB
es6-promise@3.1.2 x (52 failing) 426ms / 113MB x 10 6.3KB
native@0.12.4 x ( 4 failing) 590ms / 173MB x 13 0KB
core-js@2.1.0 x ( 4 failing) 838ms / 198MB x 11 13.9KB
es6-shim@0.34.4 950ms / 145MB x 12 130.8KB
q@1.4.1 x (47 failing) 1599ms / 425MB x 74 15.4KB

@ysmood ysmood mentioned this pull request Feb 15, 2016
@ysmood ysmood changed the title Promise yaku Port Yaku to core-js Feb 15, 2016
@zloirock
Copy link
Owner

Thanks for the PR, but I'm already not sure about adding dependencies to core-js. Some conceptions which I wanna use in the future makes it problematic. Need to think about it.

Without any tests I see some problems:

  • Subclassing in .then should use @@species pattern, but uses this.constructor, it's completely incorrect.
  • Iteration closing in Promise.all and Promise.race and some other problems with iterators.
  • Static methods have separate feature detection and polyfilling. For example, .resolve should be always replaced in library version for some reasons.
  • In environments without Symbol, well-known symbols @@iterator and @@species will not work in library version or if es6.symbol module will be required after es6.promise.
  • Support rejectionHandled event.

I tested core-js against the promises-es6-tests, it didn't pass all the tests.

I know about it, it's a part of my not public Promise test case. core-js didn't pass 2 asserts because of 1 minor bug with the order of resolving. It's a minor problem, I was going to fix it later. Other 2 asserts can fail only with full core-js version because of Number iterator - use shim version for this test case.

What do you think about replacing core-js Promise core to modified Yaku Promise core instead of adding it as a dependency?

@ysmood ysmood force-pushed the promise-yaku branch 3 times, most recently from 55a417a to 15bd031 Compare February 18, 2016 11:14
@ysmood
Copy link
Author

ysmood commented Feb 18, 2016

  • Subclassing in .then should use @@species pattern, but uses this.constructor, it's completely incorrect.

Fixed

  • Iteration closing in Promise.all and Promise.race and some other problems with iterators.
  • Static methods have separate feature detection and polyfilling. For example, .resolve should be always replaced in library version for some reasons.
  • In environments without Symbol, well-known symbols @@iterator and @@species will not work in library version or if es6.symbol module will be required after es6.promise

Fixed

  • Support rejectionHandled event.

Added

What do you think about replacing core-js Promise core to modified Yaku Promise core instead of adding it as a dependency?

No dependency now. I will merge the yaku.js to es6.promise.js later.

Any other problems?

@zloirock
Copy link
Owner

Something like that makes sense. I will explore it when I will have spare time.

@ysmood
Copy link
Author

ysmood commented Feb 19, 2016

No harry, please take your time. I will add more test cases, like these:

https://github.com/ysmood/yaku/blob/master/test/basic.js

https://github.com/ysmood/yaku/blob/master/test/basic.js

@ysmood ysmood force-pushed the promise-yaku branch 2 times, most recently from 36f5a6d to ce3cb3c Compare February 23, 2016 08:13
@zloirock
Copy link
Owner

Closed because of inactivity.

@zloirock zloirock closed this Aug 17, 2017
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