-
-
Notifications
You must be signed in to change notification settings - Fork 581
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
Converting src/browser-index.js to typescript #2016
Converting src/browser-index.js to typescript #2016
Conversation
Hello all! |
@SimonBrandner Can you please have a look and let me know if anything else is required. |
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.
Thanks for working on this!
I am not the most knowledgeable about this part of the app, so better to leave the review to someone else. A review from a team member is required anyway
@SimonBrandner Thank you for the response. |
A review from them has already been requested. You can see review requests in the top right corner |
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.
Overall it seems fine - thanks! There's a few pieces to fix up here, and some merge conflicts to consider.
enum IndexedDBStoreName { | ||
crypto = "matrix-js-sdk:crypto" | ||
} |
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.
This should be a constant
crypto = "matrix-js-sdk:crypto" | ||
} | ||
|
||
type GlobalObject = NodeJS.Global & typeof globalThis & Record<"matrixcs", typeof matrixcs>; |
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.
I thought we already had a type for this? In general I don't think we should be touching global
and should fix it.
interface RequestOptions { | ||
qs: string | Record<string | number | symbol, string>; | ||
qsStringifyOptions: IStringifyOptions; | ||
} |
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 be importing this type, not defining it.
@Aryaman1706 are you able to continue with this PR? |
This happened |
Signed-off-by: Aryaman Grover [email protected]
This change is marked as an internal change (Task), so will not be included in the changelog.