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

fire and forget option #57

Closed
wants to merge 1 commit into from
Closed

fire and forget option #57

wants to merge 1 commit into from

Conversation

reqshark
Copy link
Contributor

when the fire and forget option is true or at least not left undefined, then the constructor could be invoked without an error if you elect not to pass its callback function.

@Raynos
Copy link
Collaborator

Raynos commented Dec 12, 2014

lgtm.

@reqshark reqshark mentioned this pull request Dec 12, 2014
@naugtur
Copy link
Owner

naugtur commented Dec 12, 2014

My turn :)
First of all - I accepted the callback being optional because it follows what request does.
In request you can add an error handler to the returned object afterwards, and since we return XMLHttpRequest, it can be done in the browser too (although not with the same api unfortunately)
Swallowing errors is bad indeed, but maybe console.warn would be enough? Not sure here.

Anyway, by adding a fireAndForget option we create a new difference between request and xhr, which I consider a regression in gettint to the point where our API is an entirely contained subset of request's API.

That's why I was more ok with letting people skip the callback than with making them add a funky option if they want to use their code from node in the browser.

Does this sound reasonable to you guys?

[edit]
logging a warning makes even more sense if we decide to get our users used to getting helpful warnings from xhr - see #58

@scottcorgan
Copy link
Collaborator

I agree. Keep it as close to request as possible. Although I'm not a fan of these types of silent magic tricks, request has given us an interface to mirror.

@Raynos
Copy link
Collaborator

Raynos commented Dec 12, 2014

@naugtur

If we want to have the same interface as request we have no option but to not allow optional callbacks.

I'm fine with callback always being required.

silent exceptions are unacceptable.

@reqshark
Copy link
Contributor Author

It depends on the type of exception and where it's coming from.

I think for most network request failures, these exceptions already bubble up from the native implementation under the wrapper by virtue of XMLHTTP object opening and sending. Your browser console may also show these errors, usually without interrupting your running app client-side.

@naugtur
Copy link
Owner

naugtur commented Jan 8, 2015

Since we can't get a conclusion, and there's bugfixing and other work to do, I'm going to remove the change (make the callback obligatory again) and we can introduce something once we arrive at a decision.

@reqshark
Copy link
Contributor Author

reqshark commented Jan 9, 2015

@naugtur well hang on a second, that error we nooped out the other day has nothing to do with whether a callback is optional, it's just a syntax error.

it was confusing when i saw it at first too since i never wrote anything in my app calling apply(), so i knew it had to come from this module. if you follow that trail it just leads to a bug in the error implementation of once module, so actually I would caution against reintroducing that type of error into the likely programmable of field of this module.

@naugtur
Copy link
Owner

naugtur commented Jan 9, 2015

@reqshark Thanks for pointing that out. I'll be careful not to reintroduce the problem (unless it was caused by not passing a callback, then I'll just make sure the error message is descriptive)

@reqshark
Copy link
Contributor Author

reqshark commented Jan 9, 2015

@naugtur ya that'd be fine just wanted to keep that up on the bug killing radar you got going on here

@naugtur naugtur closed this Jun 2, 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.

None yet

4 participants