Ajax CORS fixes

[benlesh]

Working off of #6691

(added a commit to that PR, but I was unable to do that from the Web UI to that branch, so I started a new branch)

[driskell]

Hi @benlesh

In v6 the default for crossDomain was true: https://github.com/ReactiveX/rxjs/blob/6.x/src/internal/observable/dom/AjaxObservable.ts#L163

In v7 it’s code was changed yet the code read “Defaults” and said crossDomain true, it was just reading “undefined” at point of checking whether to add a header due to the default not taken into account. Tracking the changes in commits this appears to occur in an unrelated change and there was no breaking change notification so I believe it was unintentional and needs fixing.

Adding the header forces a CORS, breaking if the server does not accept the OPTIONS header.

So unfortunately this PR doesn’t fix the reported issue as it seems to enforce a new default if false for CORS - which is a breaking change from v6 to v7 I cannot find documented. I also don’t believe this breaking change is desirable.

Hope that makes sense.

[benlesh]

The more I think about this, I'm starting to feel like we're just doing this wrong.

Looking at the behaviors as they're defined.

I think that we can probably deprecate crossDomain and just "let it happen naturally", and stop setting X-Requested-With entirely. That's something a lot of libraries did, but it seems many (in particular, I looked at axios) have moved away from it.

I'm going to mark this for review with the Core Team so that can be discussed.

[driskell]

@benlesh So that the current "broken" status can be rectified is it all possible to go back to my original PR and get that pushed through as a restoring of v6 functionality for a minor point release perhaps?

I'm generally cautious about mixing change requests with bug fixes as the former can take a lot of time to discuss and agree, and in the meantime there's potential for others to have the same issues I've encountered attempting to upgrade to v7.

[benlesh]

@driskell at this point I think we need to be more cautious than to declare your PR a simple bug fix (one person's bug fix is another person's breaking change). For one thing, the original behavior in 6.x was hap-hazard, and 7.x was meant as a move to fall in line with what most other AJAX impls were doing. However, looking more closely at what other AJAX impls are doing now and what browsers do with CORS, it leads me to believe that we need to slow down and think about what needs to be done here.

1. 7.x, since release, has defaulted to crossDomain: false (even if there was a bug that said it was true in the request when it wasn't). Switching that to default to true would constitute a breaking change within version 7.x. We can't assume people are not relying on that behavior.
2. The addition of X-Requested-With seems relatively pointless, and other AJAX implementations have removed that default. However, we can't do that without constituting a breaking change. Which means we'd have to wait for 8.x to do that.
3. We may want to remove crossDomain entirely in an upcoming release, given that it's not really a simple boolean thing that dictates whether or not CORS is used. So we should probably deprecate the setting (which is non-breaking), so we can remove it later (which would be breaking).
4. The only for-sure "bug" I've seen in this is that the Request information says it defaults to crossDomain: true when in fact it doesn't.

As a workaround, people can simply add any custom header to force CORS (including X-Requested-With). My gut tells me that within 7.x, we should probably just fix the one bug above, document the issue and the workaround, and make plans to improve things for version 8.x. However, I want to confer with the core team before making that decision final.

[driskell]

@benlesh Ah ok I can completely agree and understand. I had assumed the change wasn’t deliberate as I couldn’t find it documented and the commits seemed unrelated.

In that case is it worth just updating this page to document the change? https://rxjs.dev/deprecations/breaking-changes
(Not actually sure where that’s controlled or by whom though.) And I can raise a PR to update the docs site inside this repository.

[benlesh]

Core team: Proceed with plan above.

[benlesh]

Sorry, I got a little tangled with all of what was done on top of this, so I started from scratch with another PR at #6710 that addresses the issues as discussed.

@driskell I'd really like to thank you again for bringing this to our attention. Sorry this might not be the outcome you wanted exactly, but I think it's movement in the right direction, and we'll definitely be using what was learned here to make things even better in upcoming versions.