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

Added check to allow callback be sent to options argument #128

Merged
merged 2 commits into from
May 2, 2019
Merged

Added check to allow callback be sent to options argument #128

merged 2 commits into from
May 2, 2019

Conversation

coltonehrman
Copy link
Contributor

Description

Added checks that will allow passing of callback function to 2nd options argument.

Will log error message and return function if callback function was passed to 2nd argument AND 3rd argument was passed.

References

#48

Notes

Need to look into cleaning up code base, lot of duplicate code needed.

@evanplaice
Copy link
Owner

Nice. This will work for now.

I wonder if, with the 1.0 (ie API breaking) release I wonder if it would be beneficial to move the data attribute into the options object. That would make the API work better with node's callback style.

Copy link
Owner

@evanplaice evanplaice left a comment

Choose a reason for hiding this comment

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

LGTM

@coltonehrman coltonehrman merged commit 126361b into evanplaice:master May 2, 2019
@coltonehrman coltonehrman deleted the fix/optional-options branch May 2, 2019 15:27
@coltonehrman
Copy link
Contributor Author

Yes, also was thinking of possibly breaking code up into modules for version 1.0. Make it easier to maintain and organize.

@evanplaice
Copy link
Owner

I'm a bit conflicted about that. Not because it isn't a great idea (ie it is) but because I'd like to lock this in at 1.0 and start a new clean room implementation that leverages the latest JS best practices.

I created #129 to outline the specifics

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