Remove dependency on dojo/streams

rorticus commented 7 years ago

We want to remove the dependency on dojo/streams, since it is a lot of code for as little as it will be used. Right now its only used in request/node.

One possible approach is to use dependency injection...

request/node has a dependency on dojo/streams right now. That’s pulling in a lot of code for a feature that most people will not be using. We need to remove that dependency and instead force people to manually dojo/streams if they want to use the node request provider with a streamTarget.

A desired usage would be something like,

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

// inject stream start / completion handlers
supportNodeRequestStreams(nodeProvider);

// inject node request provider
request.setDefaultProvider(nodeProvider);

request.get('http://www.example.com', {
    streamTarget: myStream
}).then(...)

Create some properties on node in request/node to allow setting a stream target handler and a stream target completion handler.
Take the code that deals with streams in request/node and move it out into dojo/streams somewhere.
Provide a convenience method to inject the stream target handlers into request/node in dojo/streams.

You can see an rough implementation here, https://github.com/dojo/core/compare/master...rorticus:no-streams https://github.com/rorticus/streams/compare/no-streams

bryanforbes commented 7 years ago

I like where this is headed, but I don't like exposing streamHandlers from dojo-core/request/node. What if that was made a property of NodeRequestOptions, then dojo-streams/request could wrap dojo-core/request/node and pass in streamHandlers for the user:

export function request(url, options) {
    return nodeRequest(url, {
        streamHandlers: {
            streamDataHandler: nodeStreamDataHandler,
            streamTargetHandler: nodeStreamHandler,
            streamCompleteHandler: nodeStreamCompleteHandler
        },
        ...options
    });
}

rorticus commented 7 years ago

I like that. So dojo-streams would import dojo-core and not the other way around.

bryanforbes commented 7 years ago

Right, and there wouldn't be an object left out in the open (streamHandlers) that could get hijacked

rorticus commented 7 years ago

PR is over here in dojo-streams, https://github.com/dojo/streams/pull/6

rorticus commented 7 years ago

Finished with https://github.com/dojo/streams/pull/6

dojo / core

Remove dependency on dojo/streams #252