-
Notifications
You must be signed in to change notification settings - Fork 4
use StrictUnion instead of NonDiscriminatedUnion for better Auth options #51
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
Conversation
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.
👍 Looks good to me! Reviewed everything up to b62818d in 1 minute and 7 seconds
More details
- Looked at
78lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. packages/fetch-links/links/authLink.ts:1
- Draft comment:
Organize import statements by grouping type imports and regular imports separately, and order them alphabetically for better readability. - Reason this comment was not posted:
Confidence changes required:10%
The import statements inauthLink.tsare not organized properly. It's a good practice to group and order imports for better readability.
2. packages/fetch-links/links/authLink.ts:26
- Draft comment:
Ensure that only one authentication method is used at a time. If multiple auth options are provided, the current logic will overwrite previous headers. Consider adding validation to enforce a single auth method. - Reason this comment was not posted:
Comment did not seem useful.
3. packages/fetch-links/links/authLink.ts:29
- Draft comment:
Ensure thatbtoais available in the runtime environment, as it is not natively available in Node.js. Consider using a polyfill or alternative method if necessary. - Reason this comment was not posted:
Confidence changes required:50%
Thebtoafunction is used for encoding credentials in base64. This is generally fine for basic auth, but it's worth noting thatbtoais not available in Node.js environments without a polyfill.
Workflow ID: wflow_8h0hZ9dHMZV5A8EX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
👍 Looks good to me! Incremental review on 2d442a6 in 9 seconds
More details
- Looked at
475lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. sdks/sdk-openint/src/index.spec.ts:327
- Draft comment:
Consider removing or addressing the TODO comment to ensure code clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The codebase uses both single-line and multi-line comments. Consistency in comment style is important for readability.
Workflow ID: wflow_Siz8X5IVboR5FBKb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
👍 Looks good to me! Incremental review on fa50739 in 35 seconds
More details
- Looked at
69lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. sdks/sdk-openint/src/index.ts:64
- Draft comment:
Consider adding a check to ensureopts.auth?.openIntis defined before spreading it into thegenerateHeadersfunction to avoid potential runtime errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code is already safe because: 1) Optional chaining is used 2) The spread operator handles undefined gracefully 3) generateHeaders only sets headers for defined values 4) TypeScript types suggest these are optional values. The comment is suggesting additional safety that isn't needed.
Perhaps there could be runtime errors if opts.auth?.openInt contains invalid types that don't match the expected header values?
The TypeScript types and the generateHeaders function's implementation ensure type safety - it only sets string values for specific known keys.
The comment should be deleted as it suggests unnecessary defensive programming when the code is already properly handling optional values.
Workflow ID: wflow_D8u8k2j6DeUu1ASY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Replace
NonDiscriminatedUnionwithStrictUnionforClientAuthOptions, refactorauthLinkheader logic, and update tests for improved type safety and clarity.NonDiscriminatedUnionwithStrictUnionforClientAuthOptionsinauthLink.ts.type-utils.tsto defineStrictUnionandNonDiscriminatedUniontypes.authLinkinauthLink.tsto use concise header setting logic.initOpenIntSDKinindex.tsto removeopenIntfromauthafter processing.index.spec.tsto validate newStrictUnionbehavior and ensure correct header precedence and handling ofauth.openInt.This description was created by
for fa50739. It will automatically update as commits are pushed.