cefn / watchable

Repo for @watchable/store and supporting packages.
MIT License
5 stars 1 forks source link

Why the weird syntax? #15

Closed cefn closed 1 year ago

cefn commented 1 year ago

In DefaultWatchable there are properties authored like this... https://github.com/cefn/watchable/blob/main/packages/store/src/lib/watchable.ts#L9

protected notify = async (item: Value) => {

Why are they not using class method syntax?

protected async notify(item: Value) {

Would be quick to check by removing this pattern throughout the codebase and proving npm run qa. Maybe the method signatures previously aligned with a function type, or this was a closure before and became a class?

cefn commented 1 year ago

This is because of https://medium.com/@joespinelli_6190/using-arrow-functions-to-avoid-binding-this-in-react-5d7402eec64 and changing it back causes a subtle regression from changing the scope of this that was only detected in store-react tests.

See also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions and although it says don't use them as methods, that's using Object method syntax, where there is no this. Later in the same document they also say...

Because a class's body has a this context, arrow functions as class fields close over the class's this context, and the this inside the arrow function's body will correctly point to the instance (or the class itself, for static fields).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

cefn commented 1 year ago

I have added some method binding tests directly to the store package. Closing.