Upgrade to xhr v2.0

[naugtur]

xhr is now fully compatible with a subset of request. Some bugs were eliminated.
It would also make #21 much easier to implement - just by replacing xhr with request. And with a minor setup the whole ampersand model would work in node.js

Now the http errors are treated as responses, so it's a change that requires a minor update to logic.
I'm planning to do a PR for that, but it'd be nice to open a conversation on that first.

[ahdinosaur]

+1, i'd love to use ampersand-sync in node.js.

[naugtur]

I'd need input from contributors on how you want the choice between request and xhr to be made.
The point here is to find a way to set it up once, so all models on top of that would just work without any boilerplate for checking if it's node or browser.

I make the choice like this:

var requestEngine = require('request')

package.json:

browser:{
  'request':'xhr'
}

but that's not an option here, as browser is the default for backward-compatibility reasons.

So the only option left is setting the xhrImplementation field.
Does anybody have a better idea?

[ahdinosaur]

as browser is the default for backward-compatibility reasons.

what do you mean by this?

another option is using nets, which takes the package.json browser field approach.

[naugtur]

I mean if you run browserify on code using ampersand-sync you shouldn't need to set anything up to get xhr as a dependency. So using the browser field in package.json to let user switch is not an option.

Also, the switch to xhr 2 will not affect the API of sync. You don't have to update major version.

[naugtur]

Can I get input from maintainers on something?
xhr used to return the XMLHttpRequest object with added fields (and a totally different thing in IE8/9). Now it's returning a plain object with all needed fields for a response, including headers hash and XMLHttpRequest in a field called rawRequest.

Should the 3rd argument to options.success be the new response object or the XMLHttpRequest object?
My initial idea was this:
var result = resp.rawRequest || resp;
But it would be nicer if the response from xhr was used.

What are your thoughts?

But if you are planning on releasing a new version

[lukekarrys]

Hey @naugtur, sorry for the delay in answering your questions.

To answer one of them, yes we are planning a major version. The open issues for it are being tracked here https://github.com/AmpersandJS/ampersand-sync/milestones/v4.0 (and this issue is a part of it).

For the others:

• I think the argument to the success handler should be the new response object from xhr (which still should have access to the raw request). We can package this as part of a major version update so I think it's a good idea to align with xhr.
• As for how to decide between request and xhr I think an option like xhrImplementation is ok, but I'd be open to other ideas (of which I currently have none 😄). Maybe some other @AmpersandJS/core-team might have some ideas?

@naugtur If you want to work on a PR for this that would be great! It would probably be a good way to generate more discussion around some of the points raised here.

[HenrikJoreteg]

rather than include configurable, I'd be tempted to pick a single library that does both. Perhaps https://www.npmjs.com/package/superagent ?

[HenrikJoreteg]

though using superagent may require too much of an API change. I'm just a bit hesitant to include two disparate libraries and hope they keep their APIs in sync.

[naugtur]

I intend to implement the whole thing. I'm asking for more discussion so that I can implement something that's inline with your expectations.

I am working on a PR, actually, I have the main change done, but I'd like to build a layer of integration tests for sync+xhrImplementation that could be run for both browser and node. That kind of test would be hugely beneficial to me right now as I'm wondering what can I break updating xhr. And if someone wishes to put anything else in place of xhr they can be sure it works.

For node/browser switching I'm looking at the browser option from package.json to get the choice of library done. I just need to check how it's resolved (does browserify take local package.json into account, or just the root? what about webpack? etc.)

@HenrikJoreteg
disclosure: I'm one of the contributors to xhr
SuperAgent is a great tool, but it has issues that matter here:

• totally different API (as you said); impact: minor
• it differs in browser support (this is controversial, I see really old activeX objects from IE6 era there, but no XDomainRequest support for IE8/9, no browser support declaration in docs); impact: hard to guess
• has a 3x bigger footprint. In browserified code the difference is 17116bytes, 5209bytes when uglified. Overall it would be a little more than +25% increase in ampersand-sync size. impact: serious

When you develop with browserify, size matters ;)

And Re: " I'm just a bit hesitant to include two disparate libraries and hope they keep their APIs in sync."
That's the whole point of xhr2.
xhr was a replacement for request from the start, and now with v2.0 we made it an exact subset of request's api and we intend to keep it that way. Also, the fundamentals of this api are quite basic and with integration tests I mentioned earlier it'd be possible to be on the ball with any number of implementations.

[lukekarrys]

I'm +1 on the points @naugtur raised and would also add that superagent currently has 125 open issues/PRs. That can be looked at a few different ways, but it does give me some hesitation about switching to it.

[naugtur]

TJ left the node community and all his projects had a hickup, hard to say how they're doing just by looking at the repo. I can guarantee support for xhr with my word ;)

[HenrikJoreteg]

@naugtur @lukekarrys good enough for me, I'm a +1.

Any other thoughts from @AmpersandJS/core-team?

[kamilogorek]

I know that @naugtur will take care of xhr in case anything changes, so I'm also +1 for this :)

[latentflip]

+1 on this, would help with serverside stuff. @naugtur what's the status of the implementation? I'm happy to give it a shot if you're busy?

I tried just upgrading to xhr@2, and the tests pass, but not sure what to look for in more detail. Is there a summary of what's changed in 1->2?

[naugtur]

@latentflip I've been sick recently and I lagged. I can push what I already wrote to my fork, but there's the response format change involved and someone with more experience using ampersand state and models could try assessing the consequences.

As for the tests - if I understood them correctly, current tests are stubbing the xhrImplementation, so there is no way changing xhr dependency could affect results. Integration tests are also in my definition of done for this issue ;)

[latentflip]

@naugtur yeah, that's what I was worried about with the tests. No pressure, just curious :) If you're up for pushing to a fork so I can take a look that'd be 👍

[naugtur]

I pushed the minimal change required to work with xhr2 to https://github.com/naugtur/ampersand-sync and it's pretty trivial

but,

Instead of XMLHttpRequest it now passes a response object to the callbacks.

{
    body: Object||String,
    statusCode: Number,
    method: String,
    headers: {},
    url: String,
    rawRequest: xhr
}

xhr1.x used to extend XMLHttpRequest with fields, but it was both: breaking older browsers and incompatible with request module that xhr was supposed to mimic.

If someone is calling methods on it or inspecting it for request headers to log them as error related data - it's going to break.

1. Is that something Ampersand users could do?
2. Should we care if it's a major version update? (afterall the request object is still available, but in a field)

[HenrikJoreteg]

I'd say we do a major version bump. Versions are cheap.

[latentflip]

Though I'd add, that majoring ampersand-sync is annoying, so if we can avoid it let's do that, if not let's make sure we check other issues and roll any other stuff into it.

[naugtur]

In that case we need to sacrifice some of the browser support (breaking changes would remain in ie8/9 but nowhere else) to keep it backwards-compatible. But it means that the same code would work differently in ie8/9 than everywhere else. Doesn't sound like a good idea.

On the other hand, xhr since something like 1.5 was only supporting old ies with this different response object (switched on by a certain option) so it was a pain in the a... already.

You guys look around if you want to bump the version number and what to put there, I'll focus on integration tests for now.
Do you think using mocky.io for testing is ok, or is it a problem that the tests would depend on something that is not localhost?

[latentflip]

If mocky is easy and reliable I would start there, if it doesn't work out we can surely port to something else easy enough?

Philip Roberts
&yet

On 25 Feb 2015, at 22:34, Zbyszek Tenerowicz notifications@github.com wrote:

In that case we need to sacrifice some of the browser support (breaking changes would remain in ie8/9 but nowhere else) to keep it backwards-compatible. But it means that the same code would work differently in ie8/9 than everywhere else. Doesn't sound like a good idea.

On the other hand, xhr since something like 1.5 was only supporting old ies with this different response object (switched on by a certain option) so it was a pain in the a... already.

You guys look around if you want to bump the version number and what to put there, I'll focus on integration tests for now.
Do you think using mocky.io for testing is ok, or is it a problem that the tests would depend on something that is not localhost?

—
Reply to this email directly or view it on GitHub.

[wraithgar]

Engine support is different than api support. This is a discussion we've had in the past and the last I heard it was something @HenrikJoreteg was going to ping the npm people about?

[HenrikJoreteg]

@naugtur yeah, we can sort out versioning later. Let's just get it working the way we want to first. http://www.mocky.io/ sounds good to me.

[naugtur]

I've started testing how sync integrates with xhr.
This trap took 2 hours to find:

ampersand-sync/test/index.js

Line 195 in 74e311b

xhr.ajaxSettings.error();

error was then asynchronously called after 404 came in, so if tests ran long enough, it would add a new t.end() to an ongoing test.

You can check out latest changes here: naugtur@bbdc412 (some comments there)

I'll poke around how ajaxConfig passing works too, because something seems missing or broken there.

BTW, why does it require underscore instead of using a selection of Amp?

[HenrikJoreteg]

The underscore requirement is just old. We simply haven't swapped everything out yet. As a side note, much of my personal time to work on ampersand related stuff has been eaten up in the last month or so because i'm finishing up another project. I anticipate having more time to put into it personally in the coming weeks.

[EvanCarroll]

Not sure where this attempt left off, but the ship is sailing on this. Rendr is now going to address the need to run Backbone on the client and server. I can't get either Render, Ampersand, or Backbone's .save working yet under Node. Good luck on this patch. +1.

• http://stackoverflow.com/questions/30358221/how-do-i-test-backbone-models-methods-that-delegate-to-ajax
• http://stackoverflow.com/questions/30361900/how-do-i-use-rendrs-models-rendr-shared-base-model-under-node
• http://stackoverflow.com/questions/30362859/running-backbone-model-or-something-like-it-with-sync-save-fetch-on-the-server-u

Just so people/search engines know, firing this up under node prior to xhr 2.0 yields

 TypeError: xhr.open is not a function
  at Object.createXHR [as xhrImplementation] (/home/ecarroll/node_modules/ampersand-model/node_modules/ampersand-sync/node_modules/xhr/index.js:61:9)

[naugtur]

Well, I'm waiting for someone to take a look at my initial minor change and respond to some comments I made there. As far as I know the change is still going to happen, but no idea when.

[wraithgar]

Sorry @naugtur which minor change is this? Is it in ampersand or xhr?

[naugtur]

I pushed the minimal change required to work with xhr2 to https://github.com/naugtur/ampersand-sync and it's pretty trivial

and

You can check out latest changes here: naugtur/ampersand-sync@bbdc412 (some comments there)

The naugtur@bbdc412 link being the commit with comments I wanted to discuss.

[wraithgar]

@bear have you seen mocky.io and do you have any thoughts about the comments in that link about in re having something useable for tests only during the tests vs something that's Just Available?

[wraithgar]

Other than that one question that PR looks imminently mergeable. Really really great work there @naugtur I'm a big fan of the test refactor!

[naugtur]

Thanks @wraithgar
My last concern is - could anyone confirm that we reached an agreement on the shape of result object returned? (the issue with the new response from xhr2 and what it used to be)
We could make it backward-compatible at the cost of IE8 support as far as I remember. (not sure about ie9)

[wraithgar]

We only claim IE9+ on ampersandjs.com so I don't foresee the IE8 support being any kind of a real sticking point.

[EvanCarroll]

Can't we just document how to use an XHR polyfill on IE8, it is possible -- right?

[naugtur]

Well, the issue is, xhr does support IE8 and if we want to maintain backwards-compatibility for sync, we'll have to add a line to it that will always throw an error in IE8 unless you override global XMLHttpRequest constructor to return a POJO.

That's the shortest summary of the problem I'm capable of delivering ;)

[bear]

@wraithgar I love mocky.io because it allows tests to standup callbacks as needed

[EvanCarroll]

Are we seriously holding up a release because an underlying version potentially adds support for an older browser that wasn't supported? This is not good practice. ampersand-sync hasn't seen a release in months. This bug report affects me too. Coming here to see if assistance was needed on this topic, I see the conversations have already been had. The bug report was opened over five months ago!

For a system that sells itself on modularity there are clear problems in the release structure.

My advice would be to push out a release ASAP, we have 9 people participating in this report. Shy of that, my advice would be to follow the semvar suggestions.

• Make this 4.0.0-alpha-1 (push to npm).
• Update Ampersand.Model's package.json to require 4.0.0 in ampersand-sync.
• Push out a new Ampersand.Model with ${nextvar}.0.0-alpha-1 .

We've got a bad case of indecision here, let's get this show on the road. Support for IE8 (a positive) is delaying me testing my amp.model in a headless system. Typically, we'd open up another bug for the new version talking about how to handle versions we don't want to support, right? That seems like a separate issue entirely (and, the way everyone handles that is to YMMV-it silently).

[naugtur]

IMHO what I proposed (28Feb) is ok, as it would be consistent across browsers and node, with rawRequest field containing the native XHR object in the browser. Since then I'm waiting for decisions I don't want to make as I'm not a maintainer.

This thread seems to confirm that the integration tests I've written are ok.
This tread seems inconclusive in regard to change in the response object.

I'll just make a pull request and let you guys decide, ok?

[EvanCarroll]

👍 I think that's a better start then waiting for months for more discussion about consistency with IE8.

[EvanCarroll]

As a side node, I'm not 100% sure I opened this bug for related functionality in the right place, but we may want to examine it here to, and potentially also in the scope of xhr2.js

AmpersandJS/ampersand-model#50