Convert to 2.2 class mixins - patches, queries, and storage #109
Conversation
Codecov Report@@ Coverage Diff @@
## master #109 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 17 17
Lines 1396 1393 -3
Branches 255 253 -2
=====================================
- Hits 1396 1393 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Not the most thorough review, but it looks solid. There are a few things I don't quite understand yet (e.g. why we still have things like createSort and createFilter), but I assume there's a reason that I'm not following yet, so I think it has my approval. You may want to wait for someone else to approve as well.
| @@ -56,7 +56,8 @@ export const loaderOptions = { | |||
| { name: 'dojo', location: 'node_modules/intern/browser_modules/dojo' }, | |||
| { name: '@dojo', location: 'node_modules/@dojo' }, | |||
| { name: 'sinon', location: 'node_modules/sinon/pkg', main: 'sinon' }, | |||
| { name: 'rxjs', location: 'node_modules/@reactivex/rxjs/dist/amd', main: 'Rx.js' } | |||
| { name: 'rxjs', location: 'node_modules/@reactivex/rxjs/dist/amd', main: 'Rx.js' }, | |||
There was a problem hiding this comment.
Do we still need rxjs here? Probably not related to this PR, but might as well clean it up since this PR is cleaning up many things...
There was a problem hiding this comment.
No we don't need it. Good catch I'll clean that up.
| @@ -21,7 +19,15 @@ function delayOperation(operation: Function, operationName: string) { | |||
| }; | |||
| } | |||
|
|
|||
| const createAsyncStorage = createInMemoryStorage.mixin({ | |||
| const createAsyncStorage = compose(InMemoryStorage).mixin({ | |||
There was a problem hiding this comment.
interesting that this now needs compose?
There was a problem hiding this comment.
This is a temporary hack as I haven't updated this test implementation(on this branch). It's just wrapping the class in a call to compose to make it a factory so it can be extended.
|
@dylans To your general comment about @agubler Any thoughts on the above? |
|
I think you're right regarding the need for a broader discussion about the general patterns used across Dojo 2 post moving to Classes from compose. As for simple factory functions, I actually don't mind them as a pattern - sometimes using classes is probably unnecessary (this is kind of proved by the fact that they were not compose factories anyway). We have functions in widget-core like |
Type: feature
Description:
Converts part of the stores repo to use TypeScript classes instead of
dojo/compose. Doesn't replace any mixins so 2.2 class mixins are not used.Relates to: #108
Please review this checklist before submitting your PR: