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

#99 allow customization of request configuration #168

Closed
wants to merge 13 commits into from

Conversation

ahamid
Copy link

@ahamid ahamid commented Dec 28, 2015

...either statically or on per-request basis (addresses #99)

This should allow customization of the dojo/request object (for example, to set withCredentials: true) either statically or on a per-request basis via extension.

Not currently tested as I couldn't figure out what was needed to satisfy the mock requests for request/rest stores, but I'm willing to add tests if I can get the harness working.

Kenneth G. Franqueiro and others added 12 commits June 24, 2015 18:03
This also (temporarily) replaces invocations of QueryMethod(...) with
QueryMethod.default(...) in existing JS source so it can still be tested.
Fixes dojo#131 by making the default comparison function created in
SimpleQuery#_createSortQuerier treat undefined as greater than any
value, and null greater than any value besides undefined.
This conveniently places all null and undefined values at
the end of a sorted list.
Fixes dojo#124 by wrapping the return value from StoreAdapter's
fetchRange function in a promise. This function is delegated
to by fetch so this handles both cases. Also updated several
test cases that were relying on the synchronous behavior of
the StoreAdapter.
The `package.json` now accuretly reflects the `LICENSE` that is included with the package.
@ahamid
Copy link
Author

ahamid commented Dec 28, 2015

actually this may be a more generic form of #87

@ahamid
Copy link
Author

ahamid commented Feb 3, 2016

any interest in merging this PR folks? this seems pretty crucial for authenticated CORS regardless of #79

@dylans dylans added the bug label Feb 3, 2016
@dylans
Copy link
Member

dylans commented Feb 3, 2016

Yes, there's interest. We'll try to get to it next week.

@@ -159,6 +166,10 @@ define([
};
},

_getRequestOptions: function(kwArgs, requestOptions, headers, requestUrl) {
return lang.mixin({}, this.requestOptions || {}, requestOptions || {});
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure the || {}s here are not necessary (mixin should default to just skipping over null/undefined things)

@ahamid
Copy link
Author

ahamid commented Feb 3, 2016

Thanks I will review and update. FWIW I discovered a similar issue with Rest which invokes xhr directly. I'm not sure if a more general solution is possible (global configuration of xhr defaults? Injection of xhr instance?)

@dylans
Copy link
Member

dylans commented Feb 3, 2016

I'm not sure if a more general solution is possible (global configuration of xhr defaults? Injection of xhr instance?)

Typically, this would be handled by creating a custom requestProvider and then usually that requestProvider globally through a configuration setting. I realize this is an area that many people find to be rough. The main reason for thinking that was a better approach was to not have dstore replicate features within dojo/request. That said, there are some obvious rough areas in dstore where we're also not respecting whatever requestOptions are provided.

@kfranqueiro
Copy link
Member

I'm not sure what you mean by "involves XHR directly", since Rest relies on dojo/request, not a specific provider directly.

@dylans
Copy link
Member

dylans commented Feb 3, 2016

I'm not sure what you mean by "involves XHR directly", since Rest relies on dojo/request, not a specific provider directly.

I assumed @ahamid meant by default in a browser, where xhr is loaded unless overridden.

@ahamid
Copy link
Author

ahamid commented Feb 3, 2016

@kfranqueiro I just meant that Rest.j invokes dojo/request itself directly, so this change to Request.js isn't automatically going to apply to Rest.js. I will look into requestProviders.

@kitsonk kitsonk added enhancement and removed bug labels Feb 8, 2016
@edhager edhager force-pushed the master branch 2 times, most recently from 1dbb25d to a5655fe Compare August 18, 2016 15:59
@dylans
Copy link
Member

dylans commented May 24, 2017

I think #206 will serve the same purpose as this one.

@msssk
Copy link
Contributor

msssk commented Jan 7, 2020

Addressed in #206.

@msssk msssk closed this Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants