Skip to content

Conversation

@lneves12
Copy link
Contributor

@lneves12 lneves12 commented Apr 23, 2025

Config example for websocket:

                neonConfig.disableWarningBrowser = false;
                const pool = new Pool({ connectionString });
                const sql = neon(connectionString, {
                  disableWarningBrowser: true
                });
image

I guess for now this should be enough, we can document it later if needed. This way is a good way to get some signal if there are a lot of people disabling it

@lneves12 lneves12 requested review from davidgomes and jawj April 23, 2025 10:52

// Ensure we are very explicit while using these apis. This ensures more type safety
// specially since this library is made to run both in the browser and node.js environments.
declare global {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was mainly to avoid adding dom types globally to the tsconfig. It makes it more type safe

@jawj
Copy link
Collaborator

jawj commented Apr 23, 2025

(1) Looks like this applies only to HTTP queries. I guess that's because those are the only ones we support auth for? But it's also possible to connect from a browser via WebSockets. Have we considered warning about that too?

(2) We should probably provide a way to suppress this message being printed, so that users can opt out of us cluttering up their console (and maybe making them look a little unprofessional in production).

@davidgomes
Copy link
Contributor

(1) Looks like this applies only to HTTP queries. I guess that's because those are the only ones we support auth for? But it's also possible to connect from a browser via WebSockets. Have we considered warning about that too?

(2) We should probably provide a way to suppress this message being printed, so that users can opt out of us cluttering up their console (and maybe making them look a little unprofessional in production).

(1) I agree we should do WS as well.

(2) I wouldn't do this, I think the point is for it to shock people. If someone really pressures us into doing this we can consider it later, shouldn't be hard.

@lneves12
Copy link
Contributor Author

  1. agreed as well. I added the warning to websocket as well. (I haven't tested the websocket part, but I will later, it should just work)

  2. I don't have a strong opinion about this one. It might be a bit annoying since it is printed in every execution, but I guess that's the goal.

@jawj
Copy link
Collaborator

jawj commented Apr 23, 2025

It might be a bit annoying since it is printed in every execution

Yeah: I see your point, @davidgomes, but I do think I'd want a way to opt out. Maybe a neonConfig option? Note that one place this warning will appear otherwise is in the console for our own SQL Editor!

One other comment: out of an abundance of caution, perhaps we should also test for typeof console !== 'undefined' && typeof console.log !== 'undefined' before logging?

@davidgomes
Copy link
Contributor

It might be a bit annoying since it is printed in every execution

Yeah: I see your point, @davidgomes, but I do think I'd want a way to opt out. Maybe a neonConfig option? Note that one place this warning will appear otherwise is in the console for our own SQL Editor!

I don't think we should a config option until someone asks for it. I'd rather avoid adding surface area and complexity unless someone specifically complains about it. More configs means more docs, more tests, and more surface area to maintain.

One other comment: out of an abundance of caution, perhaps we should also test for typeof console !== 'undefined' && typeof console.log !== 'undefined' before logging?

Good idea!

@lneves12
Copy link
Contributor Author

If the warning appears in our SQL editor that's a bit concerning, so I tend to agree that having a way to disable would make sense. This warning doesn't apply to that use case

@davidgomes
Copy link
Contributor

davidgomes commented Apr 23, 2025 via email

@lneves12 lneves12 force-pushed the lneves/warning_browser branch from 3a89152 to 2f5490d Compare April 25, 2025 00:19
@jawj
Copy link
Collaborator

jawj commented Apr 25, 2025

Sorry, just two more suggestions:

  1. We don't know that people are using RLS, right? So While your database is protected by Row-Level Security (RLS) ... should perhaps be Even if your database is protected by Row-Level Security (RLS) ....
  2. I think disableBrowserWarning or disableWarningInBrowsers would sound a lot more natural to me than disableWarningBrowser.

@davidgomes
Copy link
Contributor

Sorry, just two more suggestions:

  1. We don't know that people are using RLS, right? So While your database is protected by Row-Level Security (RLS) ... should perhaps be Even if your database is protected by Row-Level Security (RLS) ....
  2. I think disableBrowserWarning or disableWarningInBrowsers would sound a lot more natural to me than disableWarningBrowser.

Agreed on both points!

@lneves12
Copy link
Contributor Author

Sorry, just two more suggestions:

  1. We don't know that people are using RLS, right? So While your database is protected by Row-Level Security (RLS) ... should perhaps be Even if your database is protected by Row-Level Security (RLS) ....
  2. I think disableBrowserWarning or disableWarningInBrowsers would sound a lot more natural to me than disableWarningBrowser.

agreed, good feedback. Already updated it

@lneves12 lneves12 force-pushed the lneves/warning_browser branch from fc00001 to c4e3be4 Compare April 26, 2025 10:49
readOnly: neonOptReadOnly,
deferrable: neonOptDeferrable,
authToken,
disableWarningInBrowsers,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the option is not specified here, do we inherit from the global configuration parameter? I think we probably should.

Copy link
Contributor Author

@lneves12 lneves12 May 7, 2025

Choose a reason for hiding this comment

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

I guess it makes sense. The implementation is a bit weird, because neonConfig in reality is a socket config that we don't use for the http client, but I guess this is good enough (already updated)

Copy link
Collaborator

@jawj jawj left a comment

Choose a reason for hiding this comment

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

LGTM!

@jawj jawj merged commit 915f90f into main May 7, 2025
6 checks passed
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.

4 participants