-
Notifications
You must be signed in to change notification settings - Fork 214
Avoid mutating the global Promise #135
base: master
Are you sure you want to change the base?
Conversation
It has a minimal Promise interface.
src/environment.js
Outdated
Object.keys(Promise).filter(isNotMethod).forEach(del(Promise)); | ||
toFastProperties(Promise); | ||
toFastProperties(Promise.prototype); | ||
const PromiseCopy = Object.assign({}, Promise); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. This is not supported until Node 4.
Is there preferred method to checking in a polyfill?
Oh, boy. It's really red. |
These are fast. Do we need them to be faster?
I decided to remove any modifications to the bluebird promise. I wasn't able to justify modifying the promise class -- it was probably done for speed? strict interface? I intuit that there are better ways to achieve either of those. |
It's done to ensure a strict interface; portable code never uses bluebird methods off of the global Promise. |
I'm thinking we can do a few things then:
Would appreciate your thoughts on this. |
Bluebird is faster in v8 (node/chrome), but is slower in other browsers - so it's great to use in hypernova on the server, but it shouldn't ever be shipped to a browser environment. That's part of the reason that you'd want to avoid relying on it in your browser code. If there were a way to use/wrap a distinct |
Good to know!
I'll try implement something with a strict interface. Unfortunately, it seems that various Webpack plugins already use the extensive Bluebird interface. |
Why would webpack plugins run in the same context as hypernova? |
In my development environment, I am spinning up a Webpack process that wraps Hypernova as a means of having the most up-to-date Concretely:
While in the production environment, the Is there a better way to go about setting up a Webpack + Hypernova environment? |
I just realized that the set up is broken because I am spinning up a new Hypernova process on file changes. But a few tweaks and we're still relatively good-to-go. Updated setup:
|
I went ahead and implemented I need to backfill the tests for the static methods. I also dropped EDIT: coverage is missing an appropriate glob for running the new test file under |
…cond parameter to `then`
e.g. utils/strict-promise-test.js
…unning `isStrictEqual` is not part of the API: replaced with `strictEqual` Ensure promise is either rejected or resolved correctly. This was entirely missed because I forgot to ensure the tests were running. I was breaking the test suite by removing code from StrictPromise -- but not ensuring the strict-promise-test was failing appropriately.
This is certainly a cleaner approach; but I'm worried this will preclude the very performance optimizations that have us using Bluebird in the first place. |
Yeah, I had the same thought cross my mind. Are there some benchmarks we can run to get some data behind these ideas? |
This was a recent implementation by me. Figured it should not live in Hypernova if it is actually useful -- the speed of Bluebird but a small (read: strict) interface. airbnb/hypernova#135
I ran the doxbee benchmark from Bluebird. Here are the results (paraphrased for the important bits):
|
I'll give it a few more stabs and see if I can optimize it. |
The fastest I could get it down to was ~800ms @ 100mb. Though, something interesting to note: I switched out the
Bluebird is only really ~20% faster than Native ES6 Promises. The original benchmarks are skewed in favor of Bluebird because it is using a non-ES6 interface. In the case of Hypernova, I think it stands to reason that there is only a 20% performance gain over native. I might peruse Bluebird's repo for smart tips and tricks but it might not be possible to wrap Bluebird without incurring a significant performance loss (>50%) -- might fork and extract. I'll try to fix my problems by experimenting with Webpack + Hypernova parallel processing for the time being. Though, I am still interested in exploring the idea of not mutating Bluebird for the sake of code quality and instead accomplish the same feat via other methods (e.g. linting?). Let me know how you would like to proceed. |
FWIW, I ran into this same issue today trying to set up a Hypernova server based on my Rails
The comment here is what first led me down this path. Looks like I understand it isn't a common situation to be loading webpack alongside Hypernova, but I'd still love to see some kind of resolution on this. |
Fixes #134