Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Simplify core/request #253

Closed
rorticus opened this issue Dec 13, 2016 · 4 comments
Closed

Simplify core/request #253

rorticus opened this issue Dec 13, 2016 · 4 comments
Assignees
Milestone

Comments

@rorticus
Copy link
Contributor

Currently the request method will dynamically load the default provider, depending on if you are running in node or the browser. This adds some complexity to the code and probably isn’t even needed.

We’re looking for a usage like this,

import request from 'dojo-core/request';
// import node support manually if we want to use it
import nodeProvider from 'dojo-core/request/node';

// set the default provider some how
request.setDefaultProvider(nodeProvider);

// the next request will go to node
request.get(...);
  • Remove the dynamic loading from ProviderRegistry and just assume that defaultValue is already a provider, not a string.
  • Add a method to set the default provider, so it can be injected.
  • Create a convenience method to inject request/node as the default provider for people running in Node.

You can see a rough implementation (as well as some unrelated changes...) here,

master...rorticus:no-streams

@dylans dylans added this to the 2016.12 milestone Dec 13, 2016
@dylans dylans modified the milestones: 2017.01, 2016.12 Dec 21, 2016
@sebilasse
Copy link

Just my 2 cents : I like the dynamic loading.
It is consistent with all modules which you can use either in the browser or node.js ...
e.g. /text, /util, /queue, /on have switches for node/browser and as a user
I would expect to use any module without external requirements.

@matt-gadd
Copy link
Contributor

@sebilasse We would still support dynamic loading (even when in most environments it is definitely more desirable to make concrete at build time), it just wouldn't happen in the core of request itself. Currently the code to dynamically require down the browser path is super ugly - we queue requests while loading the provider, this kind of thing doesn't seem like it should be a request concern.

@matt-gadd
Copy link
Contributor

Following on: In the past, the above complications would've been removed by dealing with this in the dependency block in AMD using the has plugin to conditionally require the environment specific module. But given we're consciously trying to keep as format neutral for the future we're better placed on making these kind of module loading decisions outside of request. The point being that the environment will/should be known prior to a request being made.

@rorticus
Copy link
Contributor Author

Fixed in #261

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants