Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

it's not possible to set async to false #5

Closed
ecarnevale opened this Issue Jul 27, 2011 · 12 comments

Comments

Projects
None yet
3 participants
Member

ecarnevale commented Jul 27, 2011

my bad…

Owner

ntoll commented Jul 28, 2011

It shouldn't be possible to set async to false. It's a bad idea to make AJAX requests with async set to false. We shouldn't let them do it. I'd rather we not allow them to hang themselves in this way. :-/

@ntoll ntoll closed this Jul 28, 2011

Member

ecarnevale commented Jul 28, 2011

I (politely) disagree :)
It's a lib, why should not be possible?

JavaScript Ajax call can be sync or async (apart from jsonp or cross-domain), but why not?

I did use them in the past.
Default was true, the bug is that if you set it to false is still true.

Sent from my iPhone

On 28/lug/2011, at 18:47, ntollreply@reply.github.com wrote:

It shouldn't be possible to set async to false. It's a bad idea to make AJAX requests with async set to false. We shouldn't let them do it. I'd rather we not allow them to hang themselves in this way. :-/

Reply to this email directly or view it on GitHub:
#5 (comment)

Owner

ntoll commented Jul 28, 2011

That's my point: all requests using this library will be via CORS so it makes no sense to allow them to try to set it..! :-)

Member

ecarnevale commented Aug 8, 2011

I re-open this issue, the bug is still there (I've the fix, re-ordering the test-suite).

@ecarnevale ecarnevale reopened this Aug 8, 2011

@ecarnevale ecarnevale was assigned Aug 8, 2011

Owner

gridaphobe commented Aug 8, 2011

Pardon my js noob-ness, but why shouldn't we be able to set async to false? I'm doing that in the browser extensions for lastpage.me and it seems to work fine. I need it to be synchronous so I can issue a DELETE call and then a PUT, but embedding a function within a function within a function looks god-awful...

Owner

ntoll commented Aug 8, 2011

I'm not here, but since I'm passing through here's why you should never set async as false: it blocks the main browser thread, causing a degradation in responsiveness and potentially interrupting the rendering of the page. This is a bad situation and should be avoided at all costs. Far better to "chain" functions together in an event driven fashion. I realise you might think this is f*ugly but that depends on how you write it! ;-)

There are various ways to solve this problem in an elegantly event driven way. I'll leave it as an exercise for the reader to work out how. Hint: I've solved this problem by recurring on an array of functions where the result of each function is passed into the next function in the array.

Member

ecarnevale commented Aug 8, 2011

@gridaphobe, I totally agree with you. Thing is the code in the lib has a
bug: if you set async to false it sets it to true (wasn't supposed to work
like that :P ) it's just a || true where it was supposed to be an if
undefined then set to true :)

On Mon, Aug 8, 2011 at 9:46 PM, gridaphobe <
reply@reply.github.com>wrote:

Pardon my js noob-ness, but why shouldn't we be able to set async to false?
I'm doing that in the browser extensions for lastpage.me and it seems to
work fine. I need it to be synchronous so I can issue a DELETE call and
then a PUT, but embedding a function within a function within a function
looks god-awful...

Reply to this email directly or view it on GitHub:
#5 (comment)

Owner

gridaphobe commented Aug 8, 2011

@ntoll, I see your point, but I wonder if that's also applicable to browser extensions, where the request is already being executed in a "background" page. It seems odd that background work like that should be able to block the main thread, but regardless, it sounds like my current code is both unidiomatic and unlikely to be working the way i expect, so I'll revise it.

Thanks for the clarification :)

Owner

ntoll commented Aug 8, 2011

Sure, your extension will be executed in a separate thread so this problem of blocking the browser's main thread definitely doesn't apply to your specific case (I was thinking of the problem from a traditional web dev point of view). I suspect it'll block your extension though. :-)

Owner

ntoll commented Aug 13, 2011

Guys, I've just taken another look at this and will fix with the following behaviour"

  var fi = fluidinfo({username: "username, password: "password"});
  var options = {
    path: "namespaces/ntoll",
    args: {
      returnDescription: true,
      returnTags: true
    },
    async: false
  };
  var result = fi.api.get(options);

Why do it like this..? Two reasons:

  • The specification of XmlHttpRequest says that is async = false then the onreadystatechange callback isn't called (and it's in here that we call onSuccess or onError) so we only have the choice of returning a result rather than raising an event.
  • It's more natural looking code for synchronous blocking functions to return results.
Owner

ntoll commented Aug 13, 2011

Please review here:

#42

@ntoll ntoll added a commit that referenced this issue Aug 13, 2011

@ntoll ntoll Merge pull request #42 from fluidinfo/5-allow-async-false
Merges in fix for issue #5: Allow async=false requests :-(
bc0920d
Owner

ntoll commented Aug 13, 2011

Fixed, reviewed and merged.

@ntoll ntoll closed this Aug 13, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment