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.

Sign up for GitHub

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

Jump to bottom

react-packager: Switch from Q to Bluebird as promises library #516

Closed
wants to merge 6 commits into from
Closed

react-packager: Switch from Q to Bluebird as promises library #516

wants to merge 6 commits into from

Conversation

pilwon
Copy link
Contributor

@pilwon pilwon commented Mar 30, 2015

This PR improves performance of react-packager by switching the promises library from the Q to Bluebird.

Here is the test result showing a noticeable difference. (2x speed improvement)

Please refer to this issue for more details.

@pilwon
Copy link
Contributor Author

pilwon commented Mar 31, 2015

Jest is not running on my machine for some reason. Let me fix it and see where the test fails. (Jest doesn't run on Node.js v0.12...)

It turns out code works fine but Jest gets confused with bluebird. (blocker issue: jestjs/jest#90)

@petkaantonov Jest works with Q but not with bluebird. Do we have a solution for this?

@pilwon
Updated jest.dontMock from q to bluebird for DependencyGraph. Test st…
dd861d4 
…ill fails because of issue in Jest/Bluebird.
@pilwon pilwon mentioned this pull request Mar 31, 2015
Closed
@amasad
Copy link
Contributor

amasad commented Mar 31, 2015

ok, I think I can fix the jest error (or at least hack around it). I'll work on pulling this

@amasad
Copy link
Contributor

amasad commented Mar 31, 2015

Tested on internal source (which is considerably larger) and this yields a consistent 30% speedup. Great job! I'll sync the changes here soon.

@pilwon
Copy link
Contributor Author

pilwon commented Mar 31, 2015

@amasad 30% speedup sounds beyond awesome. I have a feeling a lot of folks will enjoy the speed improvement with this update ✨😀👍

Btw, I noticed promisify() is the only non-ES6 standard promises feature used in DependencyGraph. Not sure about the other modules but if the use of promises library can be kept to minimum like that we may want to search for an even better alternative. (Bluebird is still my No.1 choice but you never know...)

@pilwon
Copy link
Contributor Author

pilwon commented Apr 1, 2015

@amasad Updated all code to use bluebird. q is completely dropped. (even removed from package.json) Passed all tests.

@amasad
Copy link
Contributor

amasad commented Apr 1, 2015

I already updated that internally :) I should've mentioned that

@amasad
Copy link
Contributor

amasad commented Apr 1, 2015

I just need to sync from our internal repo. Our tools are not in a great place now, but we can't merge pull requests directly from github we have to go through internal review process

@pilwon
Copy link
Contributor Author

pilwon commented Apr 1, 2015

@amasad Understood. FB's merge policy between the internal code and react-native was not clear to me even after reading this section.

Will FB always update its internal code first then bring the changes back to react-native after? This means PRs cannot be directly merged to react-native that is usually the only repo external contributors have access to. I can see the benefits of this merge strategy, such as the quality of contributed code is guaranteed with FB's internal review process (I assume FB has a great one) and code consistency can be easily achieved. As long as there isn't much delay until a PR makes its way back to react-native I am okay with it.

If this is the case though, please add more information to CONTRIBUTING.md so all contributors who send PR clearly understand the merge policy. Thanks-

@pilwon pilwon mentioned this pull request Apr 1, 2015
Closed
@vjeux
Copy link
Contributor

vjeux commented Apr 1, 2015

@pilwon we want to eventually be github first but are not ready yet. For the time being, we need to ensure that every pull request passes our internal test suites and don't break our internal apps. Good idea about updating the contributing.md file

@sahrens
Copy link
Contributor

sahrens commented Apr 1, 2015

Merged internally and synced.

@sahrens sahrens closed this Apr 1, 2015
@pilwon pilwon deleted the packager-q-to-bluebird branch April 1, 2015 07:16
@pilwon pilwon restored the packager-q-to-bluebird branch April 1, 2015 07:16
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 13, 2015
@pilwon
[react-packager] Switch from Q to Bluebird as promises library
a8f8fa1 
Summary:
This PR improves performance of `react-packager` by switching the promises library from the [Q](https://github.com/kriskowal/q) to [Bluebird](https://github.com/petkaantonov/bluebird).

[Here is the test result](facebook#361 (comment)) showing a noticeable difference. (2x speed improvement)

Please refer to [this issue](facebook#361) for more details.
Closes facebook#516
Github Author: Pilwon Huh <pilwon@gmail.com>

Test Plan:
./runJestTests
start app and click around
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 14, 2015
@pilwon
[react-packager] Switch from Q to Bluebird as promises library
be3a1a4 
Summary:
This PR improves performance of `react-packager` by switching the promises library from the [Q](https://github.com/kriskowal/q) to [Bluebird](https://github.com/petkaantonov/bluebird).

[Here is the test result](facebook#361 (comment)) showing a noticeable difference. (2x speed improvement)

Please refer to [this issue](facebook#361) for more details.
Closes facebook#516
Github Author: Pilwon Huh <pilwon@gmail.com>

Test Plan:
./runJestTests
start app and click around
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 15, 2015
@pilwon
[react-packager] Switch from Q to Bluebird as promises library
87599bf 
Summary:
This PR improves performance of `react-packager` by switching the promises library from the [Q](https://github.com/kriskowal/q) to [Bluebird](https://github.com/petkaantonov/bluebird).

[Here is the test result](facebook#361 (comment)) showing a noticeable difference. (2x speed improvement)

Please refer to [this issue](facebook#361) for more details.
Closes facebook#516
Github Author: Pilwon Huh <pilwon@gmail.com>

Test Plan:
./runJestTests
start app and click around
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
@pilwon
[react-packager] Switch from Q to Bluebird as promises library
7f6255b 
Summary:
This PR improves performance of `react-packager` by switching the promises library from the [Q](https://github.com/kriskowal/q) to [Bluebird](https://github.com/petkaantonov/bluebird).

[Here is the test result](facebook/react-native#361 (comment)) showing a noticeable difference. (2x speed improvement)

Please refer to [this issue](facebook/react-native#361) for more details.
Closes facebook/react-native#516
Github Author: Pilwon Huh <pilwon@gmail.com>

Test Plan:
./runJestTests
start app and click around
jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020
@sgny @MoOx
Add Event API documentation to the index (facebook#516)
bdbdce9
This was referenced Sep 12, 2021
Open
Open
Open
This was referenced Sep 13, 2021
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pilwon @amasad @vjeux @sahrens