-
Notifications
You must be signed in to change notification settings - Fork 88
feat: dynamically inferred core types #995
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: 22.1
Are you sure you want to change the base?
Conversation
8af3ebf
to
8a70534
Compare
📝 Documentation updates detected! You can review documentation updates here |
…ertokens:supertokens/supertokens-node into feat/dynamically-inferred-core-types
{ | ||
userId: input.userId, | ||
userIdType: input.userIdType, | ||
externalUserIdInfo: input.externalUserIdInfo, | ||
externalUserIdInfo: input.externalUserIdInfo || null, |
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.
Why did this change?
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.
In the API spec we define the possible types for externalUserIdInfo
to be string | null
but the SDK defines them as string | undefined
. Do you reckon I change the SDK type definition instead? Or the API spec?
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.
@sattvikc what effect does each option have? null clears the mapping, right?
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.
Leaving replies
lib/ts/core/versions/5.3/schema.d.ts
Outdated
@@ -3,6 +3,11 @@ | |||
// @ts-nocheck | |||
// @ts-nocheck | |||
// @ts-nocheck | |||
// @ts-nocheck |
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.
Why is this added so many times?
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.
Done
{ | ||
userId: input.userId, | ||
userIdType: input.userIdType, | ||
externalUserIdInfo: input.externalUserIdInfo, | ||
externalUserIdInfo: input.externalUserIdInfo || null, |
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.
@sattvikc what effect does each option have? null clears the mapping, right?
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.
Leaving replies
Summary of change
This PR adds support for inferring the querier request body and response body types based on the path entered. It uses the core driver interface spec file to achieve this.
There is a script that generates the updated schema and can be run with the following command, whenever necessary (i.e. whenever the API spec is updated):
Related issues
Test Plan
All tests should pass
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)
Checklist for important updates
coreDriverInterfaceSupported.json
file has been updated (if needed)lib/ts/version.ts
frontendDriverInterfaceSupported.json
file has been updated (if needed)package.json
package-lock.json
lib/ts/version.ts
npm run build-pretty
recipe/thirdparty/providers/configUtils.ts
file,createProvider
function.git tag
) in the formatvX.Y.Z
, and then find the latest branch (git branch --all
) whoseX.Y
is greater than the latest released tag.add-ts-no-check.js
file to include thatsomeFunc: function () {..}
).exports
inpackage.json