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

Add chunk.stream to allow streamables in context #617

Merged
merged 5 commits into from
Apr 14, 2015

Conversation

sethkinast
Copy link
Contributor

When Dust finds a streamable in context, it begins capturing the streamable's data events and renders the block once for each thunk. If the streamable is a reference, Dust writes the thunks directly into the chunk.

Other fixes as part of this PR;

  • Fix async tests in the Node runner
  • Allow functions in Dust context to return functions recursively
  • Escape data returned directly from a Thenable (e.g. { foo: PromiseFor("<evil>"); }

Note: highland.js (browser version) is included as a test lib because the highland repo only exports the Node version as part of its NPM package.

Closes #616 by reducing the needed code to:

var recordStream = mydbconn.query('...').stream();
var ctx = {
   records: recordStream
};
dust.stream('myTemplate', ctx); 

Also respects filters, so {promise|s} unescapes properly
@sethkinast
Copy link
Contributor Author

Reduces the code in the streaming example to:

app.get('/', function (req, res) {
  dust.stream("hello", {
    "async": request('http://www.dustjs.com/')
  }).pipe(res);
});

sethkinast and others added 2 commits April 8, 2015 11:00
When Dust finds a streamable in context, it begins capturing the streamable's `data` events and renders the block once for each thunk. If the streamable is a reference, Dust writes the thunks directly into the chunk.

Other fixes as part of this commit;
* Fix async tests in the Node runner
* Allow functions in Dust context to return functions recursively

Note: highland.js (browser version) is included as a test lib because the highland repo only exports the Node version as part of its NPM package.
* Stream#emit returns `true` if listeners exist
* Stream#pipe requires the sink to at least have `write` and `end` methods
* Stream#pipe tries to emit a `pipe` event on the sink
* Stream#pipe listens for `error` events from the sink and stops piping to it
* Stream#pipe does NOT try to invoke `error` methods of the sink, because real Streams never have an
  `error` method.
* Because of the way Dust handles chunk#setError, rendering aborts as soon as an errored chunk
  occurs. So, when a stream emits an 'error', it also emits 'end' (even though not all Streams work
  this way, Dust streams do)

This also finally got me to rewrite the render tests, which were a bit of a mess.
@sethkinast
Copy link
Contributor Author

I've now added more to this PR. Dust Streams were really not very similar to real Streams, especially in the way that they implement pipe. I've made them more like ReadableStreams.

  • Stream#emit returns true if listeners exist
  • Stream#pipe requires the sink to at least have write and end methods
  • Stream#pipe tries to emit a pipe event on the sink
  • Stream#pipe listens for error events from the sink and stops piping to it
  • Stream#pipe does NOT try to invoke error methods of the sink, because real Streams never have an
    error method.
  • Because of the way Dust handles chunk#setError, rendering aborts as soon as an errored chunk
    occurs. So, when a stream emits an 'error', it also emits 'end' (even though not all Streams work
    this way, Dust streams do)


var destEnded = false;

if(typeof stream.emit === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we combine the two if's and make isStreamable something that is on-able and emit-able?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good comment / observation.

It's basically readable vs writable streams. For the purposes of dust.isStreamable you need anything that emits events (and hopefully the events contain data and end). This could even be some custom thing you wrote yourself-- as long as it has an on method, Dust can use it as a ReadableStream.

But to pipe to a Stream, it needs to be a WritableStream so it needs to have write and end methods. The emit and on parts allow piping to conform to Node WritableStreams spec, but they're optional-- for a Stream to be pipable for Dust, it only needs write and end.

So it isn't sufficient to check isStreamable for piping, because you really need a WritableStream, not just any Stream.

If we wanted to restrict the kinds of Streams you can use I'm cool exploring that-- I just tried to make Dust as lax as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense. asking for consistency rather then for more features. i agree though keeping it lax make sense.

The only thing (very nit picky) is then the choice of the name chunk.prototype.stream. Would calling it stream be confusing? await makes sense in promises-land... maybe awaitStream? idk. very nit picky so i'm cool with what's currently here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just Dust convention TBH, I don't mind what we use.

But most of them are already nouns like reference, section, block. Steven asked for something other than thenable so we went with await.

Stream is a verb and noun so it makes sense either way? :D

@sethkinast sethkinast modified the milestone: 2.7 Apr 9, 2015
prashn64 added a commit that referenced this pull request Apr 14, 2015
Add `chunk.stream` to allow streamables in context
@prashn64 prashn64 merged commit 98dda84 into linkedin:master Apr 14, 2015
@sethkinast sethkinast deleted the chunk.stream branch April 15, 2015 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to render an object stream context variable rather than an array?
3 participants