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.

Sign up for GitHub

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

Jump to bottom

Fix #1714 conditionally halting stream #2200

Merged
merged 4 commits into from
Aug 8, 2018
Merged

Fix #1714 conditionally halting stream #2200

merged 4 commits into from
Aug 8, 2018

Conversation

porsager
Copy link
Contributor

@porsager porsager commented Aug 8, 2018 •
edited
Loading

Description

When a stream returns halt it is unregistrered to avoid calling dependent streams

Motivation and Context

#1714

How Has This Been Tested?

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)
  • Documentation 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

barneycarroll
barneycarroll suggested changes Aug 8, 2018
View reviewed changes
Copy link
Member

@barneycarroll barneycarroll left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Stick something in the changelog?

@porsager porsager requested a review from tivac as a code owner August 8, 2018 13:46
@barneycarroll barneycarroll merged commit d64e0a9 into MithrilJS:next Aug 8, 2018
@pygy
Copy link
Member

pygy commented Aug 8, 2018

I didn't review this in depth, but it seems like the wrong fix to me... stream.HALT is supposed to stop the propagation of the current value, not to end the dependent streams...

The docs isn't clear about it but I think it was meant to be reversible.

@porsager
Copy link
Contributor Author

porsager commented Aug 8, 2018

That's a good point @pygy , but I don't think the word HALT conveys that.. I think IGNORE / SKIP would make better sense?

@barneycarroll
Copy link
Member

I'm really sorry - totally jumped the gun. You're right @pygy, HALT stops that one value - stream.end(true) is the API for that thing. I read the tests as validating a fix that meant HALTed values didn't trigger derivative streams.

The test should in fact ensure that the flag can be flipped back again.

@orbitbot
Copy link
Member

orbitbot commented Aug 8, 2018 •
edited
Loading

Re:naming #2017

Could we include that renaming part into 2.0 since it's semver major anyways?

@pygy
Copy link
Member

pygy commented Aug 8, 2018

+1 for SKIP

The stream library has its own npm package, we can introduce breaking changes independently.

@porsager
Copy link
Contributor Author

porsager commented Aug 8, 2018

@pygy that's also a great point for not including stream as a named export in the mithril esm bundle!

Do we want to do anything special at all if returning HALT? I don't think we should if we introduce SKIP which works as you defined?

porsager added a commit to porsager/mithril.js that referenced this pull request Aug 9, 2018
@porsager
Fix MithrilJS#1714 conditionally halting stream (MithrilJS#2200)
ccfcaea 
* Fix MithrilJS#1714 conditionally halting stream

* Add note in changelog
@porsager porsager mentioned this pull request Aug 23, 2018
Merged
11 tasks
@gamb
Copy link

gamb commented Nov 8, 2018

+1 for SKIP as well.

This is actually a breaking change for #1957. The tests need to be better for it.

dead-claudia pushed a commit that referenced this pull request Nov 14, 2018
@porsager @dead-claudia
output mithril, stream and ospec esm versions on build - fixes #2112 (#…
c3896b9 
…2194)

* output mithril, stream and ospec esm versions on build

* Add esm bundles

* [request] Clearer error message for JSON deserialization failure (#2195)

* Bundled output for commit fd7cf80 [skip ci]

* Fix #1714 conditionally halting stream (#2200)

* Fix #1714 conditionally halting stream

* Add note in changelog

* Do not include stream as named export in mithril.esm.js

* Rename mithril.min.esm.js to mithril.esm.min.js

* Add esm files to eslintignore

* Add named exports

* Add hyperscript `m` as named export

* Add builds with export changes

* checkout regular bundled files

* Change .esm.js to .mjs

* Update pkg module to point to .mjs

* Fix for export names to avoid collision

* Updated bundled files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barneycarroll barneycarroll barneycarroll requested changes

@tivac tivac Awaiting requested review from tivac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@porsager @pygy @barneycarroll @orbitbot @gamb