Xhr2 and node

[latentflip]

Extending #45 (merge that one first) to test node support, by switching xhr for request as per #21

Currently some tests are failing:

• ensuring that "application/json" is set in the headers
• ensuring beforeSend is called

I haven't dug into why yet.

[naugtur]

I also looked into the failing tests with application/json header and saw that the test worked only because xhr reused the object passed in as headers. If a copy of that object is used, the reference will not point to the object that actually holds the expected header. (I could make a change in xhr that wouldn't affect the API nor return values, but would break all json header assertions)

IMHO, unit tests need to replace the request maker with something that would handle assertions and integration tests should ensure that sync works with both xhr and request.

If you agree with the above, I could get it done on Saturday (if I wake up before dinner time ;) )

[HenrikJoreteg]

btw, @naugtur, really appreciate all your work on this. I've added you as a member of the community team so you can easily contribute to this PR directly. If you're ok with it, I would happily denote that on this page as well: http://ampersandjs.com/contribute

[naugtur]

That's cool, thanks! I feel like I should do a bit more and wait for 4.0 to get released before my name shows up at http://ampersandjs.com/contribute, but I'm happy to join.

It's getting late. I'll get back to this PR on Friday and I'll rebuild the tests to be independent of minor implementation details in xhr.

[naugtur]

That's it for decoupling the unit tests from 3rd party code. And I didn't have to do anything to core to make them pass (except for beforeSend, see above)

And btw, I noticed that when initially splitting the tests into unit and integration I hit the autoformatter and re-formated everything with my 4space setting. My bad. Is it worth the mess in commit history to make it consistent again?

Also, ampersand-model uses 4spaces, I'm confused. ;)

[latentflip]

This is all looking good to me, I'm not sure if we've decided on 2 or 4 space across the board yet :/

@HenrikJoreteg et al?

[HenrikJoreteg]

Currently we're on 4 spaces on all ampersand modules, we should stick to that. (should have specific lint rules, probably)

[naugtur]

Oh, so my autoformatter did a good thing. yay!

[HenrikJoreteg]

@latentflip was there something that wasn't 4 spaces or something? Anyway, I'm +1

[HenrikJoreteg]

@latentflip @naugtur not sure exactly where this left off or what's left, here. Does this all work happily in node and with both browserify and webpack now?

Also, @naugtur you're clearly very meticulous about details so I've also given you publishing rights on npm for this module. Here's a bit about how we've been doing releases: http://ampersandjs.com/learn/bug-triage-process/#publishing at the point where any of the PRs you're involved in are ready per the guidelines on that page, fire away.

[naugtur]

Thanks for the access, I'll do the reading. This change was supposed to be one of the main items in v4 of sync, so I'd rather not do my first publishing on a major version update ;)

I made the testrunner run everything on node also. Didn't test cross-browser though. I hoped someone would gather all the info about changes planned for v4 and decide to merge. That's why I wait.

I don't feel like shipping the v4 myself, so @latentflip, I hope you'll have time to finalize that... with my help if needed ;)

[naugtur]

Ok, I'm convinced ;)

[HenrikJoreteg]

hey @naugtur, so is this just waiting for someone to put this on npm? I gave you access. I believe in "open" open source :) Ship it!

http://ampersandjs.com/learn/bug-triage-process#publishing

[naugtur]

I know that, and I was meaning to say I'll do it by my previous comment here. This change was supposed to be released as 4.0 so before I release it, everything planned for 4.0 has to go in. I fixed 2 more things, merged in some stuff and there's one more thing left to implement before I can release 4.0