CHECKOUT-2507: Convert DataStore module into TypeScript#35
CHECKOUT-2507: Convert DataStore module into TypeScript#35davidchin merged 3 commits intobigcommerce:masterfrom
DataStore module into TypeScript#35Conversation
| return result; | ||
| } | ||
|
|
||
| return Object.assign({}, result, { [key]: newState }); |
There was a problem hiding this comment.
How come we're no longer using object spread here anymore? Is does TS not play well with spread?
There was a problem hiding this comment.
Apparently there's an ongoing issue with the use of the spread operator with generics. microsoft/TypeScript#13557
We are using the default configuration provided by `tslint`. We have added some overrides in order to match with the configuration we have for JavaScript files (enforced by `eslint`). We will need to revisit the rules some time in the future.
davidchin
force-pushed
the
data_store_typescript
branch
from
fe91b38 to
eb4f14c
Compare
| describe('deepFreeze()', () => { | ||
| it('throws error if mutating object', () => { | ||
| const object = deepFreeze({ message: 'Foobar' }); | ||
| const object = deepFreeze({ message: 'Foobar' }) as any; |
There was a problem hiding this comment.
Need to typecast to any in order to bypass the TypeScript compiler check. Otherwise, because deepFreeze returns Readonly<T>, you're not able to assign any new values anyway. But I want to keep the tests because I want to test what would happen in JS environment where type checking is not enforced.
There was a problem hiding this comment.
You should probably also chuck a comment about this statement with the same contents as in your GH comment...
software-artificer
left a comment
software-artificer
left a comment
There was a problem hiding this comment.
Looks good to me. Really exciting to see this TypeScript movement!
src/data-store/action.ts
Outdated
| @@ -0,0 +1,6 @@ | |||
| export default interface Action<P = any, M = any> { | |||
There was a problem hiding this comment.
It looks weird to me that we are using single letters for generic type names. I would strongly suggest having meaningful names here.
| describe('deepFreeze()', () => { | ||
| it('throws error if mutating object', () => { | ||
| const object = deepFreeze({ message: 'Foobar' }); | ||
| const object = deepFreeze({ message: 'Foobar' }) as any; |
There was a problem hiding this comment.
You should probably also chuck a comment about this statement with the same contents as in your GH comment...
| @@ -1,6 +1,6 @@ | |||
| export default interface Action<P = any, M = any> { | |||
| export default interface Action<TPayload = any, TMeta = any> { | |||
There was a problem hiding this comment.
Heh, I just commented about bad habit of naming types for generics as single letters, and here you go... you just fixed them!
| */ | ||
| subscribe(subscriber, ...filters) { | ||
| let state$ = this._state$; | ||
| subscribe( |
There was a problem hiding this comment.
Maybe we should consider aliases for this type descriptions. They seem like they could be reused and make the signature hard to read.
subscribe(subscriber: Subscriber, ...filters: Array<Filter>): Subscription {
Not sure what the rest of @bigcommerce/frontend feel about this...
davidchin
force-pushed
the
data_store_typescript
branch
from
eb4f14c to
036a2b8
Compare
|
Thanks guys. I've amended the last commit to include the suggested changes. |
|
Thanks a lot DC, really nice work! |
4 participants
What?
DataStoremodule into TypeScript.tslintWhy?
tslint. I have added some overrides in order to match with the configuration we have for JavaScript files (enforced byeslint). We will need to revisit the rules some time in the future.Testing / Proof
@bigcommerce/checkout @bigcommerce/payments