-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: new JS API #19154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: new JS API #19154
Conversation
ab15560 to
5de449e
Compare
bf8a82e to
b3d453b
Compare
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! Looks solid
| const match = returnType.match(API_METHOD_RETURN_TYPE_REGEX); | ||
|
|
||
| if (!match) { | ||
| continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably log here
| const callExpressions = method.getDescendantsOfKind(SyntaxKind.CallExpression); | ||
| for (const callExpr of callExpressions) { | ||
| const expression = callExpr.getExpression(); | ||
| if (Node.isPropertyAccessExpression(expression)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I prefer to invert & continue; to reduce nesting
| * Whether the card is New (0), Learning (1), Review (2), Relearning (3) | ||
| * @returns The card `type` property | ||
| */ | ||
| public getType(): Promise<Result<number>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return this as a union/branded type
| /** | ||
| * @returns The card ID. | ||
| */ | ||
| public getId(): Promise<Result<number>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a number of these: do we want branded types?
Or at least aliasing CardId to number
| * Gets the card remaining steps | ||
| * @returns The card `left` property | ||
| */ | ||
| public getLeft(): Promise<Result<number>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? It's been a long while, but I believe it's more complex, depending on whether the card has inter or intra-day steps
| throw new Error(`Request failed with status ${response.status}`); | ||
| } | ||
|
|
||
| const responseData = await response.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unusual to not use response.json() is there a reason? (old WebViews?)
| /** | ||
| * @param error Error message. | ||
| */ | ||
| export type Failure = { success: false; error: string }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain if this is translated or not
Do we want a (string) error code which users can Google?
| interval: number; | ||
| ease: number; | ||
| takenSecs: number; | ||
| memoryState?: FsrsMemoryState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be aware that this will likely be extended, and how we handle this
| @@ -0,0 +1,13 @@ | |||
| Copyright (year) (author) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean (year) and (author), they are part of the eslint-plugin-headers template syntax.
| @@ -0,0 +1,28 @@ | |||
| { | |||
| "name": "ankidroid-js-api", | |||
| "private": false, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this going to be published/updated/pushed to consumers?
|
We probably want a longer discussion over architecture I'd prefer this as a separate repo:
|
Purpose / Description
https://github.com/BrayanDSO/AnkiDroid-JS-API in a PR. Not sure if it's better placed in this repo or on a separate one.
This is basically the typescript source for generating ankidroid-js-api.js, js-api-endpoints.json and all the docs, used in #19152
package-lock.json is 3100 lines of the PR, so don't get too afraid
Pending tasks (for other PRs) are: