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

_.streamifyAll #226

Closed
wants to merge 6 commits into from
Closed

_.streamifyAll #226

wants to merge 6 commits into from

Conversation

maxgurewitz
Copy link
Contributor

At the suggestion of @LewisJEllis I've written a preliminary streamifyAll implementation, the highland equivalent of bluebird's promisifyAll.

On top of the included unit tests I did some manual integration tests with mongoose and I didn't run into any problems.

It might be worth adding options for custom filters, streamifiers, and suffixes as offered in bluebird.

Thanks

- test
- useful for utilizing libraries like mongoose
- streamifies function properties
- streamifies constructor prototype function properties
- motivated by use with mongoose
}
} catch (e) {}
};
while (Object.getPrototypeOf(curr)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how big a deal it is but IE8 doesn't support Object.getPrototypeOf. According to the package.json file we're supposed to support that browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know I need to use Object.getPrototypeOf or __proto__ in order to support streamifying inherited methods, both of which are es5 features. However, I do check for es5 support (which as you point out IE8 is missing) and I don't call this function if we're missing it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I misread that bit at the end!

@vqvu
Copy link
Collaborator

vqvu commented Feb 17, 2015

First off, this is very cool.

I won't have time to look in-depth at this (so someone else should!), but a few comments.

First, can you rebase against the current master? Your changelist looks a little weird. For instance, why is 2dcdc6e part of the changes?

Second, can you move the helper functions to global scope? Put them in a streamifyAll.js if you have to (we need to start splitting up index.js anyway. It'll make it easier (for me anyway) to read the function, and prevent unnecessary closure creation.

@maxgurewitz
Copy link
Contributor Author

My preference is to have the helper functions in the global scope of streamifyAll.js and any shared helpers in a utils.js. I'll try and refactor/rebase later today!

@vqvu
Copy link
Collaborator

vqvu commented Mar 1, 2015

Close in favor of #241.

@vqvu vqvu closed this Mar 1, 2015
@vqvu
Copy link
Collaborator

vqvu commented Mar 1, 2015

Oops. #242.

@svozza svozza mentioned this pull request Apr 9, 2015
@vqvu vqvu added this to the v2.5.0 milestone Apr 17, 2015
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.

3 participants