Convert several modules to TypeScript #145
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This also (temporarily) replaces invocations of QueryMethod(...) with QueryMethod.default(...) in existing JS source so it can still be tested.
Fixes dojo#131 by making the default comparison function created in SimpleQuery#_createSortQuerier treat undefined as greater than any value, and null greater than any value besides undefined. This conveniently places all null and undefined values at the end of a sorted list.
Fixes dojo#124 by wrapping the return value from StoreAdapter's fetchRange function in a promise. This function is delegated to by fetch so this handles both cases. Also updated several test cases that were relying on the synchronous behavior of the StoreAdapter.
| constructor(filterArg: string); | ||
| constructor(filterArg: () => any); | ||
| constructor(filterArg: FilterArgs); | ||
| constructor(filterArg?: any) { |
There was a problem hiding this comment.
could you use a union type instead of the multiple constructor declarations?
constructor(filterArg?: string | () => any | FilterArgs | any)Additionally, not sure what our style guide says about this, but this definition will accept anything as a parameter. I can see the value of explicitly defining it this way for documentation purposes but we could just get away with
constructor(filterArgs?: any)There was a problem hiding this comment.
I combined the constructor declarations using the union type, and realized that the any type wasn't actually needed.
| store.remove(1); | ||
|
|
||
| assert.deepEqual(methodCalls, ['put', 'add', 'remove']); | ||
| assert.deepEqual.bind(this, events, ['update', 'add', 'delete']); |
There was a problem hiding this comment.
What's the reason for this bind call?
There was a problem hiding this comment.
No reason, and it was hiding the fact that this test was actually broken. I've removed the bind call and fixed the test.
The `package.json` now accuretly reflects the `LICENSE` that is included with the package.
maier49
force-pushed
the
convert-request-to-ts
branch
from
6086bd9 to
b1e3995
Compare
Fixes dojo#143, Fixes dojo#138, Fixes dojo#137, Fixes dojo#136 * QueryResults has been converted to TypeScript * Filter has been converted to TypeScript * Store and tests/unit/Store have been converted. * Request and tests/unit/Request have been converted. All test cases that were previously passing are still passing, as well as the Request test cases. QueryResults needed to be modified to return an ES6 style promise, because of calls to super made by the promise returned from dojo-core/Request. This conversion in turn required many tests cases and some sources files to be updated, as they were relying on the fact that Dojo 1 style promises execute synchronously in some cases.
maier49
force-pushed
the
convert-request-to-ts
branch
from
b1e3995 to
067d9bd
Compare
|
Given that we've moved the TS version of dstore to dojo/stores, I'm closing this out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #143, Fixes #138, Fixes #137, Fixes #136, Fixes #146
All test cases that were previously passing are still
passing, as well as the Request test cases.
QueryResults needed to be modified to return an ES6 style
promise, because of calls to super made by the promise
returned from dojo-core/Request. This conversion in turn
required many tests cases and some sources files to be
updated, as they were relying on the fact that Dojo 1 style
promises execute synchronously in some cases.