-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
The monkey patch in |
addon/adapters/rest.js
Outdated
@@ -1483,20 +1520,19 @@ if (isEnabled('ds-improved-ajax')) { | |||
}; | |||
}); | |||
|
|||
adapter._ajaxRequest(hash); | |||
|
|||
this._ajax(hash); |
There was a problem hiding this comment.
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?
LGTM RE my comment on 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 |
Agree on |
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. |
addon/adapters/rest.js
Outdated
@param {Object} options jQuery ajax options to be used for the najax request | ||
*/ | ||
_najaxRequest(options) { | ||
if (najax) { |
There was a problem hiding this comment.
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’
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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... |
@tchak we discussed this PR today at the weekly and we are 👍 with it moving forward. Steps to merge:
|
c796736
to
dcc96ee
Compare
dcc96ee
to
dd2662e
Compare
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 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 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 So that's the reason we were overriding the container's Now that ED no longer uses 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. |
No description provided.