Feature proposal / user-defined request parameters / pull request #79
Comments
|
@Jens-dojo is overriding/extending the var params = { param1: 'value', param2: 'another' };
var Store = Rest.createSubclass({
_renderUrl: function () {
var url = this.inherited(arguments),
joinChar = url.indexOf('?') >= 0 ? '&' : '?';
url += joinChar + ioQuery.objectToQuery(params);
return url;
}
}); |
|
Hello, now that is much more elegant, with "ioQuery". I still feel that it might be useful to have "Parameters" it as a ready-to-go feature of the Request?! Especially as overwriting a private function should be avoided by the end-user. And it doesn't create any kind of useless overhead... I could change my code based on yours and make a new pull request ? |
|
as for overriding it's not up to me whether or not your PR will be accepted but in general, if something is already possible by some other means then unless a feature is really compelling it's unlikely to be merged. your needs can be met by a subclass so we'll see if someone else thinks your case is compelling enough to merge it. |
|
Dear Ben, I agree to all what you said. If a user can find a solution to a problem by subclassing and overriding public functions - using the documentation, that's all fine for me. But we are talking about private functions that are not (yet / never?) mentioned in the APIs / documents. So there is no chance for the end-user to find a solution to this without digging (deeply) into the source code. Jens |
|
It seems like we should remove the prefixing for all these _render* On Thu, Dec 18, 2014 at 8:18 AM, Jens-dojo [email protected] wrote:
|
|
That might be one possibility. The "headers" option exists. And when - next step - we implement the feature request for "choose between GET and POST", these methods will be handy, as you can set parameters without worrying about GET/POST. |
|
I agree with the idea of removing the underscore from the _render* functions. When I see an underscore it signals to me that I should look elsewhere for a solution. |
|
Unfortunately, due to an oversight, This issue and #68 started me thinking about a more general solution for these sorts of feature requests. I pushed an http-request-customization branch that removes the underscore prefixes from those methods, clarifies their names, and adds a number of new hooks. After the change, the following hooks are available in
My hope is that these hooks will give sufficient flexibility for less common cases like #68 and cases like this where you want to add to the query params. What do you think of these? Adding support for additional query params could be done with a small mixin: var FetchParams = declare(null, {
fetchParams: null,
renderFetchQueryParams: function () {
var queryParams = this.inherited(arguments);
this.fetchParams && queryParams.push.apply(queryParams, this.fetchparams);
return queryParams;
}
});Personally, I am not sure it is worth adding a dstore feature to support additional query params, but perhaps I am wrong and there are more people out there with similar needs. For now, I believe this is a feature that should be implemented locally in your project, as a mixin or by passing overriding methods during instantiation. What do others think about this? |
|
One note: I don't love the name |
|
my preference would be to encourage the use of existing abstractions rather than make new ones. so in thinking of what's needed here, the question to ask might be "is there a way this can already be done without making a new abstraction in dstore?" and if so, then the consideration becomes how convenient the alternative is and then only add abstractions that add significant convenience or which make something possible that was not otherwise possible. @brandonpayton i'm thinking your suggested API is now overly abstract. especially since much of it seems to repeat abstraction you can already get from a i think if you start to think about whether all this abstraction belongs on a store, you may end up thinking "we need something to encapsulate the
i agree to not add so many things to dstore but simply make things possible so that consumers can use things like subclassing or request providers to get what they may need. |
|
@brandonpayton one more thing... it's been advised quite a few times to override |
|
Huge +1 to "use existing abstractions rather than create new ones". The fact that dstore uses I imagine #68 might be solvable via custom providers as well (intercept the URL in question and change Also, RE underscore-prefixed methods, I thought it was generally accepted that these are often not so much "private" as they are "internal" and are open to being extended by subclasses. There's just a possibility that the behavior may change in the future. |
|
Hello, thank you for all the very interesting suggestions! Maybe we should close this issue as well as #68 ? Merry Christmas to all of you! |
|
@neonstalwart and @kfranqueiro, thanks for the feedback. Here's a wall of text for the holidays. :) We seem to be on the same page about not introducing needless abstractions, but I'm not sure I agree with regard to the Request/Rest hooks. After thinking through these issues and sketching out my comments, I am closer to agreeing than I was, but I'm still not there. Here is where I'm coming from. The purpose of the hooks is to present existing Request/Rest behavior in a way that can be easily overridden. The methods add no new behavior, are simple, and represent the truth of what an HTTP collection needs to do for each operation. If dojo/request providers were a good solution, I would agree that it is wasteful to expose these methods, but I believe request providers are a weak solution. My biggest concern is that dojo/request providers operate at the level of an HTTP abstraction, not at a collection level. This has the following consequences:
Some other concerns are:
What are your thoughts? |
|
Hello, I don't have the inside knowledge into dstore that you have - but as the library should also be used by "ordinary" developers, here is my feedback.
In my web app, there is for example ONE java servlet on the remote host that handles different data requests under ONE url and with different url post/get parameters. Idea: requestProvider property in the Request store object ? If not set, dstore works unchanged. if set, dstore uses the provided requestProvider Object. So users could easily subclass RequestStore and add their own request provider in that subclass. BTW: ExtJs where I come from does it that way: the stores have a "proxy" property for "proxy objects" that are similar to requests in dojo. Something like function (request, lang, arrayUtil, JSON, declare, Store, QueryResults) {
var push = [].push;
NEW: var standardRequest=request;
return declare(Store, {
.......
_request: function (kwArgs) {
kwArgs = kwArgs || {};
NEW:
var request=standardRequest;
if (this.requestProvider)
request=this.requestProvider;
......
That small change would be the solution for all of my problems, even for #68. What do you think? Jens |
|
@brandonpayton i wanted to respond directly to some of your comments but my responses are kind of terse and scattered which may make them seem hostile - don't take them that way, i'm just extremely busy right now. so first an attempt at a general response and then some specifics.
some of these semantics can be changed by customizations to the collection but, given the bullet points above, it would not be a bad practice for the request provider and the collection to be working together rather than in complete isolation. as @Jens-dojo suggests it may be possible to build up a library of these request providers for well-known data sources and people could use a dstore/Rest instance combined with one of these request providers and be up and running without any further customization. another concern i have with such fine-grained hooks is that it's veering into 2nd system syndrome. dojo/store had a very concise API but since dojo/request didn't exist at the time when the stores were written, they did not use dojo/request. introducing dstore/Request and using dojo/request should be sufficient to address the vast majority of issues that needed to be worked around with dojo/store and HTTP data sources.
the requests currently being made unambiguously represent the semantics of each operation. it's simple for a request provider to decode what is being asked for.
this is actually exactly why they are the right place to do this - HTTP abstraction should be happening in the place that is responsible for doing HTTP requests. if i was only allowed one point in my argument, this would be it.
not quite...
this is what an API is - you have clearly defined contracts that consumers and producers rely on being met.
URLs are global and since request providers are acting on URLs this is not a problem.
as i've more or less said earlier, you shouldn't have 2 collections representing the same target but if for some reason you do, then you shouldn't need per-instance overrides for the same target. (for cases like #81 i'm suggesting that
i'm yet to see a request provider that doesn't last the lifetime of an application. the behavior of external services doesn't typically change so you don't have a need to unregister a request provider. in response to @Jens-dojo, i think you could be easily convinced to love request providers. what you describe is more or less what they are except that rather than be something that you pass to an instance of the store (e.g. |
|
Dear Ben, I am still not convinced to use Global Request Providers instead of passing an individual Request Provider to a store as I suggested. If I say "I want ALL XHR to be made as POST instead of GET", then yes, I use a Global Request Provider. Why should I use a Global Request provider and have to analyse if the request is from that ONE specific store that needs special attention? Code and system logic that belong together into one Object with one Store Object and one Request Provider are no longer separated from the Request Provider Logic of other stores? Maybe I am still missing something when I think about the Global Request Providers ......?! Best wishes |
|
@Jens-dojo have you seen how you register a request provider with dojo/request/registry? I think doing that could be useful for informing your opinion. I don't know if I can say much more without repeating myself or adding confusion. I think at this point I'd like to hear from others. the tl;dr of my position is that I agree that dstore could use a few more public methods for generating requests but I'm hesitant to include APIs which overlap with request providers. |
|
Hi @neonstalwart,
ok, understood. My responses are inline.
This is why it is best to write customizations as a collection subclass. All logic for the customization is maintained in the subclass's module with no external side-effects.
Agreed. It is unlikely. My purpose in mentioning this was to show that subclassing collections is a better-encapsulated solution.
I believe you're correct about Rest making unambiguous requests. My example of ambiguity was wrong. However, developers should be customizing collections at the collection level, not using an implementation detail of dstore/Rest.
Why have to use an external thing in combination with an otherwise encapsulated collection? dstore should provide the API necessary to customize dstore. A user shouldn't have to learn that A mixin overriding Rest hooks is more straightforward to write, read, and to use.
I have had concerns about 2nd system syndrome with other parts of dstore, but I don't believe providing hooks for existing behavior enters that territory. The hooks do not force themselves upon the collection developer; they are simply available to override if desired. In contrast, asking collection developers to customize Rest collections using
IMO, the biggest risk with the hooks is that we get the granularity wrong and make simple things hard to override without reimplementing other things. That said, I believe these hooks offer a reasonable level of granularity by allowing URLs and requests to be customized independently.
The collection API clearly, unambiguously represents the semantics of each operation. With hooks, there is no need to write the code to decode anything.
dstore is about the collection abstraction and encapsulating implementation details. Developers customizing dstore requests should be working with the collection abstraction as that is all they care about. If you're customizing requests, there's no need to think about the default HTTP request; just make the request you need based on arguments to the collection API.
Yep. I was wrong about this one. I believe all Rest operations can be decoded from their HTTP request. I just don't think we should have to decode HTTP requests to send different HTTP requests.
There are two levels to this API: the collection API and the HTTP API. Those overriding the HTTP API are writing their own request contracts and shouldn't be required to know and couple their customization to the contracts they're overriding. They just need to understand the collection API because they are implementing a collection abstraction.
It's better to avoid global constructs where possible, and it is possible here. Customizing Rest using the request registry also introduces a defect vector: If a broadly-matching request provider (say, to slightly tweak all requests) is registered before a customizing request provider, the broadly-matching request provider will be called instead of the customizing request provider. One can argue that this is unlikely or probably the result of bad architecture, but what can happen will happen. And all of us make mistakes and do stupid things from time to time. Why not use a fully encapsulated solution that does not expose itself to these sorts of issues?
The problem here is that collections are supposed to abstract away the underlying details, but your application now has to register request providers to modify collections' underlying behavior. |
|
@neonstalwart, I wanted to keep things simple and direct, but my response probably could have used the same warning about terseness. |
|
I am not sure that I understand how the abstractions suggested by these new methods is really directly replacing I am definitely attracted to minimizing our surface API, and if we can encapsulate a series of steps to minimize documentation and complexity, and retain flexibility for future changes, without any significant loss in extensibility, that would be great. However, I think what @brandonpayton has proposed does represent a pretty well-hardened set of steps for translating store operations to HTTP requests. The operational steps that would be exposed are basically the same steps that existed internally throughout dojo/store/JsonRest history, and have good backing in REST semantics and HTTP specification. I wonder if there are particular methods proposed that would be more desirable to be cut? Perhaps the As far as the removal of the underscore, I feel like, from my experience, that the idea of using underscore's to denote "internal" (or "protected") instead specifically private (do not touch), has turned out to be more confusing and unhelpful. It becomes a vague combination of pushing some people away from using it (giving up the benefit of truly public) and luring others into using and making it prone to breakage for future changes (giving up the benefit of truly indicating that it is private). In fact, perhaps both these interpretations exist within this thread alone :). |
|
Personally I would vouch for keeping the underscore prefix on these methods if they are to be included. They are not part of the public API per se, as you would never expect a store consumer to call any of them. At the same time, we should still have a section documenting them as methods intended to be extensible or overridable. The fact that people assume underscore-prefixed to mean "private, never touch this" is somewhat unfortunate for cases like this, and I think we need to start making it clear that there are cases where it actually means "not meant to be called externally, but totally okay to extend". Though I suppose there is still the gray area of what degree of breaking changes are allowable in them in that case. |
|
@brandonpayton i don't want to stir this up again but in looking at #108 an extra point came to mind... if you have a server that requires you to adjust your request somehow (credentials, extra data, different method, etc) then using a request provider puts all of that for you in one place so that if some requests are going through dstore and others aren't then the request provider catches all of them for you. it looks like this is such a common thing people are trying to figure out that whatever way you decide to go, i think it's probably something to move up the list of priorities for dstore so that users clearly understand how to achieve this. if request providers are the solution to recommend then it's only a matter of documentation - it would make a good FAQ blog post for sitepen.com 😉 |
Hello,
this is the first time that I work with Git and that I try to contribute to such a project, so please be patient with me :-)
Request / REST Store.
It would be useful if it were possible to define additional ajax reqest parameters that are sent with every ajax request.
Currently, one can define additional headers, but not URL parameters.
I have made changes to the source code.
The dstore.Request
gets a new
parameters
property.
// parameters: Object
// A set of key/value pairs.
// Additional user-defined parameters to pass in all requests to the server. These can be overridden
// by passing additional parameters to calls to the store.
//
// Example:
//
// parameters:{
// myPara1:'value1',
// myPara2:'value2'
// }
The code changes are rather simpel and they work fine for me.
I would be happy if you could include that new feature.
Jens
The text was updated successfully, but these errors were encountered: