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

Rewrite stream #2207

Merged
merged 16 commits into from
Nov 27, 2018
Merged

Rewrite stream #2207

merged 16 commits into from
Nov 27, 2018

Conversation

porsager
Copy link
Contributor

@porsager porsager commented Aug 23, 2018

So after a bit of fiddling trying to solve 2 issues related to mithril streams, I ended up trying to do an implementation from scratch just to get a better understanding of how it all hooked up.

After a bit of back and forth with @pygy on gitter I think I've ended up with a simpler implementation, which at the same time fixes the bugs mentioned above.

Another small benefit is that it is 1-3x faster depending on browser for simple tests using map and compose.

These are the issues being solved by the rewrite
#1714 (PR #2200)
#2197 (PR #2201)

The first one that got merged actually behaves wrong in that it ends the stream completely (wrong fix)

There was also another issue fixed with regards to the changed streams returned in combine which required fixing a test.

At the same time I've also renamed HALT to SKIP, which feels a bit more correct relating to its function.

How Has This Been Tested?

Using all current stream tests, correcting 1 test, and adding another

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

@porsager porsager requested a review from tivac as a code owner August 23, 2018 06:13
@StephanHoyer
Copy link
Member

👍

@dead-claudia
Copy link
Member

@pygy I requested your review since you appear mentioned in the lead up to this PR.

stream/stream.js Outdated Show resolved Hide resolved
@dead-claudia
Copy link
Member

@porsager Could you add Stream.HALT as a deprecated alias, so we don't break people? (You can still define it as a getter to warn on its use.)

@porsager
Copy link
Contributor Author

Good idea @isiahmeadows ... Would you recommend a single console log on first use?

@dead-claudia
Copy link
Member

@porsager Yes.

stream/stream.js Outdated Show resolved Hide resolved
stream/stream.js Outdated Show resolved Hide resolved
docs/stream.md Outdated Show resolved Hide resolved
@nordfjord
Copy link
Contributor

This looks good to me 👍

Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

So far, everything LGTM.

@dead-claudia dead-claudia removed the request for review from tivac September 18, 2018 14:25
@dead-claudia
Copy link
Member

@pygy Could you take a look at this? (I'd rather not go without your review, since it appears you were at least somewhat involved indirectly with this PR.)

@porsager
Copy link
Contributor Author

Any chance this could go in 2.0?

@dead-claudia
Copy link
Member

@porsager Unlikely unless @pygy is okay with it as-is.

@porsager
Copy link
Contributor Author

ah ok 😊 I also forgot this is versioned by itself, so not strictly tied to 2.0

@dead-claudia dead-claudia requested a review from a team November 21, 2018 20:47
@dead-claudia
Copy link
Member

@porsager Could you fix the conflicts?

# Conflicts:
#	docs/stream.md
#	stream/stream.js
@porsager
Copy link
Contributor Author

Done ;)

@dead-claudia
Copy link
Member

@barneycarroll Does this look okay to you?

Copy link
Member

@StephanHoyer StephanHoyer left a comment

Choose a reason for hiding this comment

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

lgtm.

Besides that merge artefact, i'm fine

docs/stream.md Outdated Show resolved Hide resolved
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed/Declined
Development

Successfully merging this pull request may close these issues.

4 participants