search

Shopify / javascript-utilities

A set of utilities for JavaScript and TypeScript projects at Shopify.
MIT License
45 stars 9 forks source link

Replace implementations of autobind, memoize, debounce #16

Closed TzviPM closed 7 years ago

TzviPM commented 7 years ago

We went with lodash-decorators (including typings), because the core-decorators versions or debounce and memoize are deprecated, and the version of autobind doesn't work properly with typescript.

TzviPM commented 7 years ago

CC @lemonmade as an FYI, but maybe best not to review, since we worked on this together.

utkarshsaxena93 commented 7 years ago

Hey. Won't find time to review this. I will add another reviewer :)

ismail-syed commented 7 years ago

This means all the consumers who were using autobind() will now have to use bind(), correct? If that holds true, then this will be a breaking change to consumers. i.e a v2.0.0 bump.

I think it's time we add a CHANGELOG.md for this repo. Do you mind adding one? I guess we can leave a Pre-2.0.0 sections that has a comment mention we didn't keep a changelog pre-2.0.0

michenly commented 7 years ago

Am I reading it right that we are just importing and exporting lodash-decorators's bind, debounce and memoize ?

If this is the case, why don't we just use lodash-decorators directly in neutron and polaris instead ?

TzviPM commented 7 years ago

@ismail-syed, @autobind should still work as normal. bind is being used as the default export, so any name will do. Also, note that I export bind(), not bind, in order to keep a consistent interface with our autobind.

TzviPM commented 7 years ago

@michelleyschen I feel it's better to use our own abstraction around this for 2 reasons:

  1. It avoids a major refactoring right now
  2. It avoids many future refactorings, should we decide to change our implementation. For example, if core-decorators fixed its issues with typescript, it may make more sense to use their autobind decorator instead of the bind decorator in lodash-decorators.
TzviPM commented 7 years ago

Also, @ismail-syed, what are your thoughts about moving autobind to the decorators folder? Is there a reason it isn't there now? legacy reasons, maybe?

ismail-syed commented 7 years ago

what are your thoughts about moving autobind to the decorators folder?

I'm pretty sure this one will need consumers to update since the paths will be @shopify/javascript-utilities/decorators.

I don't know the original reason, but I think it should be there since it's used as a decorator. It might have originally been in root as a mistake.