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

feat(fromEvent): add FromEvent type export #19

Merged
merged 5 commits into from
Jan 16, 2022
Merged

Conversation

huan
Copy link
Contributor

@huan huan commented Nov 9, 2021

This PR tries to implement the discussion from #9

Copy link
Owner

@andywer andywer left a comment

Choose a reason for hiding this comment

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

Great stuff! Haven't tested it yet, but looks good.

I only reverted the version bump as a best practice (imagine having three open PRs and each bumping the version while you aren't even sure they will be released together)

README.md Outdated
This can be fixed by the following code, by replacing the `fromEvent` type with our enhanced one: `FromEvent`:

```ts
import { fromEvent as rxjsFromEvent } from "rxjs"
Copy link
Owner

Choose a reason for hiding this comment

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

Really tiny suggestion: rxjsFromEvent is quite a handful. What would you think about renaming the import alias to rxFromEvent?

It's just two letters difference, but I think it would make the code feel "friendlier".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely agree with this.

Have renamed rxjsFromEvent to rxFromEvent from both the README and the source code.

@huan
Copy link
Contributor Author

huan commented Nov 10, 2021

Great to know that you think this PR is good!

I have followed your suggestion and finished the modifications.

Please let me know if there's any other suggestion so we can continue improving it, and I'm looking forward to installing this feature from NPM! (before the RxJS v8 remove the fromEvent<T> for Node.js EventEmitter ;-)

@andywer
Copy link
Owner

andywer commented Nov 11, 2021

@huan Cool. I'm just thinking what the next steps are.

How about this: Let's already publish a pre-release version to npm (v1.5.0-rc?) from the feature branch, without updating the latest tag on npm, so you can install it manually from npm, but it's only opt-in until we merge and publish the update.

Btw, I didn't quite get this bit:

(before the RxJS v8 remove the fromEvent for Node.js EventEmitter ;-)

@huan
Copy link
Contributor Author

huan commented Nov 11, 2021

Thanks, publishing a pre-release version to npm as v1.5.0-rc is good to me, I'll test it in my project after it has been published.

Btw, I didn't quite get this bit:
"before the RxJS v8 remove the fromEvent for Node.js EventEmitter ;-)"

The fromEvent<T>() that can only work with my Node.js EventEmitter code has been flagged as deprecated, and said that it will be removed after RxJS v8:

/** @deprecated Do not specify explicit type parameters. Signatures with type parameters that cannot be inferred will be removed in v8. */

See: https://github.com/ReactiveX/rxjs/blob/ecbc6c1178b528b95c44aed5c145b4e6cf7df173/src/internal/observable/fromEvent.ts#L82

I don't know if the RxJS team has already a better solution for this, but for now, it's the only one I can use.

@andywer
Copy link
Owner

andywer commented Nov 13, 2021

@huan Sorry for the delay! Published commit e581f3a as [email protected] under tag testing.

@huan
Copy link
Contributor Author

huan commented Nov 14, 2021

I have used it and everything working as expected so far:

https://github.com/wechaty/puppet/blob/f0b28519b072586f48febb383322c5e06cc9deda/tests/from-event-type.spec.ts#L33

Thank you very much for publishing this new feature!

@andywer andywer merged commit df74abe into andywer:master Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants