added test for the ending of dependent streams

[eladzlot]

Description

Added a failing test for stream, where dependent streams fail to end.

Motivation and Context

The documentation states that the end stream is:

A co-dependent stream that unregisters dependent streams when set to true.

Turns out that currently it only unregisters these streams but does not end them.

const s = Stream()
const c = stream.map(a => a)
s.end.map(() => console.log('parent ended'))
c.end.map(() => console.log('child ended'))
stream.end(true) // should log "child ended" but nothing happens

The equivalent behavior in flyd works fine (as @spacejack kindly showed)

var flyd = require("flyd")
const s = flyd.stream()
const c = s.map(x => x)
c.map(v => {console.log('child:', v)})
s.end.map(() => {console.log('parent ended')})
c.end.map(() => {console.log('child ended')})
s.end(true)
c(1) // no emit

How Has This Been Tested?

This pull request adds a test that shows this.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have updated docs/change-log.md

[tivac]

Remove package-lock.json from this PR please, it's not necessary for the PR and adds noise.

[tivac]

dependent

[pygy]

@eladzlot I'm not necessarily against this, but it clearly is a breaking change. The docs also say:

Ended state

A stream can stop affecting its dependent streams by calling stream.end(true). This effectively removes the connection between a stream and its dependent streams.

Nowhere does it say that the children will also be ended.

Now this may be an oversight in the design—IIRC @lhorie mostly mirrored how flyd works—but he may also have had a reason in mind to do it the way it is...

AFAICT A fix would be simple: replace those three lines with stream._state.deps[id].end(true)...

[dead-claudia]

@pygy There are times you might not want to end the children (e.g. merging into other streams), but they're few and far between, and in pretty much all of them in my experience, you're better served just updating them manually.

[pygy]

We could make this a non-breaking change (technically, a less breaking change :-)) by keeping the current behaviour for s.end(true) and introduce s.end({propagate: boolean}).

s.end(true) could be deprecated later on.

[spacejack]

Personally I'd be in favour of maintaining compatibility with flyd wherever possible so it could be used as a drop-in replacement. I don't know if the behaviour of end is part of the Fantasy Land spec. @JAForbes any thoughts on this?

[dead-claudia]

@spacejack

I don't know if the behaviour of end is part of the Fantasy Land spec.

It's not even defined anywhere in that spec, so there's nothing to address here.

[JAForbes]

@spacejack As @isiahmeadows says, flyd's FL interop is just of, ap and map.

I'm personally in favor of this, I think ending a parent should end all children.

[pygy]

Since streams have their own NPM package, we could make a breaking [email protected] release...

[StephanHoyer]

Unfortunately this PR is heavily outdated.

Also the stream implementation has been rewritten (#2207) by @porsager. Maybe he already solved this issue?

[dead-claudia]

@StephanHoyer PR's outdated, but the behavior didn't change.

[dead-claudia]

Closing in favor of #2315.