0no-co / wonka

🎩 A tiny but capable push & pull stream library for TypeScript and Flow
MIT License
709 stars 29 forks source link

Indicate that `this` is not used, or convert functions to arrow functions #123

Closed Tigge closed 1 year ago

Tigge commented 2 years ago

A few of the functions in Wonka triggers the @typescript-eslint/unbound-method lint rule when used. See https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/unbound-method.md.

For example, the makeSubject example from the documentation:

import { makeSubject } from 'wonka'
const subject = Wonka.makeSubject();
const { source, next, complete } = subject;

Changing export function makeSubject<T>(): Subject<T> { to export function makeSubject<T>(this: void): Subject<T> { should be enough to indicate that this is not used in these functions. Or it could just be an arrow function instead in which case we do not need to indicate that.

kitten commented 2 years ago

Hiya :wave: While I'd love to accept the issue and the corresponding PR, I'm failing to determine a reason to, sorry!

To keep it short, a random ESLint rule, that is not being applied to this repository is unfortunately not a reason to accept this issue and PR. Because no function in wonka is actually using an execution context, whether the function ends up being called bound or unbound is irrelevant. In fact, it would be pretty odd to call it bound to any given context, as there's no relevant need to. It simply does not use prototype inheritance in any sense. This is why the difference is merely in coding style.

What's also published in the final package output, is basically transpiled & compiled code, as per the Rollup configuration: https://github.com/0no-co/wonka/blob/main/rollup.config.js As part of this, it's a transpilation detail of whether the functions end up being arrow functions, function expressions, or function declarations. However, writing them as function declarations has the advantage of often not being transpiled away and retaining their name even when minified. This is nice for debugging in Node environments, where sourcemaps are often not used. However, in any browsers, even with a bundler, sourcemaps are still used and make the difference between styled negligible.

I hope that makes sense to you 🙌

Tigge commented 2 years ago

Fair enough. I can appreciate that. That linting rule is also not included in the recommended set so I doubt there will be many with this "issue". The other approach with this: void probably makes a little bit more sense in this context but then again it mostly would cater to this linting rule.

There are some more subtle use for having this: void, it would also prohibit usage of this within the functions. There are mentions of this in the old ts docs, https://www.typescriptlang.org/docs/handbook/functions.html#this-parameters - but nothing in the new ones as far as I can see. Won't add much - but yeah, let me know if you could accept something like that, if not, feel free to just close this of course.

Thank you for your time 🙌