Fix: at now curried + some addition to documentation #677
Conversation
| @@ -6,6 +6,7 @@ | |||
| | [hold](https://github.com/mostjs/hold) | Deliver the most recently seen event to new observers | |||
| | [create](https://github.com/mostjs/create) | Imperatively push events into a Stream | |||
| | [dom-event](https://github.com/mostjs/dom-event) | Streamlined DOM Events | |||
| | [adapter](https://github.com/mostjs/adapter) | Imperatively put events into mostjs streams | |||
docs/api.rst
Outdated
| ) | ||
| .finally(() => sink.end(ct(scheduler)); | ||
|
|
||
| return { dispose: () => undefined }; |
There was a problem hiding this comment.
It'd be great to show a real disposable here, e.g. one that prevents the call to sink.event/error/end above.
There was a problem hiding this comment.
I have changed the example using setTimeout now with dispose.
packages/core/src/index.ts
Outdated
| <A>(time: Time, value: A): Stream<A> | ||
| <A>(time: Time): (value: A) => Stream<A> |
There was a problem hiding this comment.
Oh jeez, I'm sorry. It didn't occur to me in #676 that this might be a breaking change for JavaScript users. I just noticed it now that I see these type signatures.
In the non-curried version, at's TS type requires the second parameter, i.e. the value, but a JS caller could omit it, e.g. at(time), and it would return a Stream.
By currying it, the JS behavior will change: instead of at(time) returning Stream, it will return a function (value) => Stream 😕
We'd need to release it as 2.0.0 to signal the breaking change. It's not a blocker, though, since I don't really care about the "marketing value" of version numbers. I figured it was worth calling out, though.
That might be reason enough to separate this into 2 PRs: one for the non-breaking changes, and one for the breaking change to at.
Thoughts on all of that?
There was a problem hiding this comment.
Ok I understand, I too missed that this is a breaking change.
I think, currying at is a too small change that it alone not justifies releasing a "2.0".
I reverted that commit, so that this PR now only deals with the non-breaking changes to the documentation.
I could author another PR for at, but rather I'd somehow bookmark the commit as "2.0-planned" to include later if the real need for a version "2.0" should arise.
Perhaps you could mark the issue #676 with a label like "2.0"?
There was a problem hiding this comment.
I've updated the signature for at in the documentation to reflect it's signature as uncurried binary and unary function.
There was a problem hiding this comment.
Perhaps you could mark the issue #676 with a label like "2.0"?
Great idea! Done.
packages/core/test/source/at-test.ts
Outdated
| @@ -12,4 +12,11 @@ describe('at', () => { | |||
| return collectEventsFor(t, at(t, expected)) | |||
| .then(eq([{ time: t, value: expected }])) | |||
| }) | |||
| it('is auto-curried', function () { | |||
There was a problem hiding this comment.
It's ok not to test this. The types already guarantee it. For example, here's the type error we'd get if we wrote this same code without at being curried:
| @@ -286,7 +286,8 @@ at | |||
|
|
|||
| .. code-block:: haskell | |||
|
|
|||
| at :: Time -> a -> Stream a | |||
| at :: (Time, a) -> Stream a | |||
| at :: Time -> Stream void | |||
There was a problem hiding this comment.
Unfortunately, the second signature is only valid for JavaScript. The TS types currently prevent calling at with just 1 argument.
Maybe we can keep only the first signature, and add a note for JavaScript users?
| at :: Time -> Stream void |
| sink.end(ct(scheduler)); | ||
| }; | ||
| const timer = setTimeout(publishVal, delay, value); | ||
| return { dispose: () => { clearTimeout(timer); } }; |
fixes #676
and docs
newStreamand adds PACKAGES.md to Readme.mdJust a quickly jotted down PR.
Happy to change/correct anything.
:-)