Skip to content
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

Fix ambiguous error on startup #944

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alexgleason
Copy link

@alexgleason alexgleason commented Sep 16, 2024

Fixes #923

Before & after:

Screenshot from 2024-09-15 18-58-17

patrickReiis

This comment was marked as outdated.

@martinwepner-otto
Copy link

Would be nice if this could be merged and released soon. In my team multiple developers ran into this when trying to connect with wrong credentials and the "cannot read property "replace" of undefined" was not very helpful.

@fin-dogedev
Copy link

This runtime type error causes the application to terminate if the connection to the database fails unexpectedly. As a result, the reconnection mechanism cannot take effect.

@coder-se
Copy link

Is there anything blocking this from being merged? Would be really good to get this into a release. The issue has been around since beginning of this year causing applications to terminate like @fin-dogedev says.

Any thoughts @patrickReiis , @porsager ?

@chanon
Copy link

chanon commented Oct 24, 2024

Please merge this.
Is this project still being maintained?

@porsager
Copy link
Owner

there are no tests so that means I need to spend time to make them and verify the fix myself - I'm not on a big surplus of time at the moment, and don't use deno myself, so can't easily verify the pr myself.

@porsager
Copy link
Owner

also changes should only be made to the src, the rest of the files are auto generated

@chanon
Copy link

chanon commented Oct 24, 2024

FYI it also happens in the Node.js version I am using v20.13.1

Also it is an uncaught exception that kills the process, so not sure how to write a test for it. I guess spawn/exec and read the error output? The fix is very simple though so not sure a test is required.

In addition to the code changed, also see the root of the issue noted here:
#923 (comment)

@wataruoguchi
Copy link

wataruoguchi commented Nov 11, 2024

During development, I had the same issue as #778. I can reproduce the issue with my repository.

  • Prod runtime: Cloudflare Workers
  • Dev runtime: Node (v22.3.0)
  • Framework: Remix
  • Database: Supabase
  • Query Builder: Kysely

I confirmed the change in the src/connection.js (Node.js) in this PR has solved the issue.

I support this PR because the queryError function expects query to be an object. If the query is NOT an object, the queryError function fails anyway. I hope this PR gets merged.

Side note: During development, we need to use NodeJS's module by adding an alias.

@alexgleason
Copy link
Author

also changes should only be made to the src, the rest of the files are auto generated

I updated to only include changes to src.

@alexgleason
Copy link
Author

I do not know how to reproduce an SSL connection error in a test. This issue is apparently also occurring in Node #923 (comment)

@alexgleason
Copy link
Author

@porsager This is a simple change that is 1 line of code and makes intuitive sense because it is just a guard clause. Will you please merge it?

@porsager
Copy link
Owner

I'll take a look at it tonight @alexgleason !

@coder-se
Copy link

coder-se commented Dec 19, 2024

We have been running a fork of this PR in production for awhile now, and there seems to be another case where query is actually is an object. But it doesn't have the origin property causing the same issue to be thrown. I think it would be good to adapt the code so that origin is only appended if the property exist.

Additionally it might good safe-guard for null as well, since typeof null === 'object'

@alexgleason
Copy link
Author

@coder-se I've never encountered that, but since it does seem possible for query to be null (eg here:

, query = null
) I added a check for it.

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.

TypeError: Cannot read properties of undefined (reading 'replace')
8 participants