Skip to content

Conversation

tejasbadadare
Copy link
Contributor

Summary

Rationale

How has this been tested?

  • Current tests cover my changes
  • Added new tests
  • Manually tested the code

Copy link

vercel bot commented Sep 2, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
api-reference Ready Ready Preview Comment Sep 5, 2025 10:47pm
proposals Ready Ready Preview Comment Sep 5, 2025 10:47pm
5 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
component-library Skipped Skipped Sep 5, 2025 10:47pm
developer-hub Skipped Skipped Sep 5, 2025 10:47pm
entropy-explorer Skipped Skipped Sep 5, 2025 10:47pm
insights Skipped Skipped Sep 5, 2025 10:47pm
staking Skipped Skipped Sep 5, 2025 10:47pm

if (isBrowser()) {
// Browser: Add token as query parameter
const urlObj = new URL(url);
urlObj.searchParams.set("ACCESS_TOKEN", token);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is available as a query param, why use bearer auth at all? Seems like if you have to support query param auth it's simpler to just always use the query param and never set the header...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeahh, considered that, but we prefer people use the header whenever possible to avoid leaking the token in logs/some nginx somewhere/etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that's fair

function addAuthentication(
url: string,
token: string,
wsOptions: any = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general you shouldn't ever use any; here you should type this correctly to match the types that are valid as options here. If you must, you can use unknown, but I don't think that makes sense in this case.

@tejasbadadare tejasbadadare marked this pull request as ready for review September 5, 2025 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants