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

Use najax in fastboot and some refactoring to isolate jQuery.ajax #5385

Merged
merged 2 commits into from Apr 2, 2018

Conversation

tchak
Copy link
Member

@tchak tchak commented Mar 16, 2018

No description provided.

@tchak
Copy link
Member Author

tchak commented Mar 16, 2018

The monkey patch in ember-cli-fastboot should be ignored after this change. We can not remove it right now because we need to support older ED versions.
The monkey patch in ember-fetch should continue to work as it replaces the whole ajax method right now. I will work on a transition story for ember-fetch next.

@@ -1483,20 +1520,19 @@ if (isEnabled('ds-improved-ajax')) {
};
});

adapter._ajaxRequest(hash);

this._ajax(hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think _makeRequest is available outside of feature flag, is it?

@runspired
Copy link
Contributor

runspired commented Mar 17, 2018

LGTM

RE my comment on _makeRequest, I think we can delete that code once the feature flag is deleted, may not require any changes now bc of that, although the change is trivial and either can happen first.

I like that you've done this in a way that avoids altering any existing private methods in ways that would be difficult for consuming apps to overcome that required using private APIs to access ajax.

I do wonder if we shouldn't go all the way to ember-fetch here though. Is a quick rundown of the API differences that keep us from doing so handy?

@tchak
Copy link
Member Author

tchak commented Mar 17, 2018

Agree on _makeRequest. I just added the code to make the tests pass in the mean time. If your PR is merged before mine, I will update.
I have another PR coming with ember-fetch support. I need to figure out a proper way to deprecate AdapterFetch Mixin.

@kratiahuja
Copy link
Contributor

kratiahuja commented Mar 18, 2018

What is the motivation behind adding najax and fetch support in ember data itself for Node? I am not sure if it is a good idea to leak other implementation support details into ember data. The reason I ask is because there could be many apps that aren't going to use fastboot and I am not sure if they should be getting all this as part of ember data itself.

@param {Object} options jQuery ajax options to be used for the najax request
*/
_najaxRequest(options) {
if (najax) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be typeof najax !== ‘undefined’

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@rwjblue
Copy link
Member

rwjblue commented Mar 19, 2018

@kratiahuja

The idea here is that this PR uses only public APIs that might be present from ember-cli-fastboot, and doing that allows us to remove the ember-data monkey patches that live in ember-cli-fastboot. Ultimately this should reduce coupling and future brittleness...

I do think there is something nice we can do to reduce the code size in the case where ember-cli-fastboot is not present at all (thinking of adding an additional flag to @pzuraq’s ember-compatibility-helpers), but I’d like to do that in a follow up...

@runspired
Copy link
Contributor

@tchak we discussed this PR today at the weekly and we are 👍 with it moving forward.

Steps to merge:

  • fix Rob's requested changes above
  • add to this PR or follow up with a second PR that uses ember-compatibility-helpers to detect if fastboot is present. This will allow us to strip unneeded code for many folks. ember-compatibility-helpers will itself need updated to have a fastboot checker.

@tchak tchak force-pushed the use-najax-in-fastboot branch 2 times, most recently from c796736 to dcc96ee Compare March 29, 2018 12:45
@runspired runspired dismissed rwjblue’s stale review April 2, 2018 20:01

has been completed

@runspired runspired merged commit b6433cd into emberjs:master Apr 2, 2018
@ryanto
Copy link
Contributor

ryanto commented Jan 17, 2019

Oh neat, happy to see this landing in Ember Data!

I know I'm very late to this party, but we were relying on the the monkey patched version that ships with ember-cli-fastboot. Specifically, we were counting on ED using the ajax:node function from the container.

We run our application on heroku, which runs app servers behind load balancers that do SSL termination. By the time a request gets to our fastboot app server the protocol is http. This means that when Ember Data builds URLs for data fetching they will be http URLs. Unfortunately, this creates a problem for us: Our fastboot app starts making Ember Data requests as HTTP.

Our api server does not allow HTTP requests, it will 301 them to an HTTPS url. In the browser this works fine because XMLHttpRequest will transparently follow redirects, but in fastboot najax does not follow redirects. Ember Data starts trying to parse a 301 as a data response and that throws an error. It results in a fastboot crash.

So that's the reason we were overriding the container's ajax:node function. We changed ember-cli-fastboot's najax to use the X-Forwarded-Proto to build the same URLs the client would.

Now that ED no longer uses ajax:node I want to update our code to a solution that makes ED do what I want without overriding private adapter APIs. I think the solution here is to use a computed property for the adapter's host in FastBoot. Does that sound right?

Also, should I be relying on najax, or is this a good time to invest in ED+fetch? Would love to hear what you all use in your fastboot+ED apps.

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

5 participants