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

feat: Adjust allowed error codes for detecting node:sqlite #3900

Merged
merged 2 commits into from
Dec 1, 2024

Conversation

xconverge
Copy link
Contributor

I am not really sure how to write a test for this...

This relates to...

N/A

Rationale

This was a bit too restrictive for how big of a failure it can lead to

Changes

Features

N/A

Bug Fixes

Fixes #3899

Breaking Changes and Deprecations

N/A

Status

@metcoder95 metcoder95 changed the title Adjust allowed error codes for detecting node:sqlite feat: Adjust allowed error codes for detecting node:sqlite Nov 29, 2024
@metcoder95 metcoder95 requested a review from mcollina November 29, 2024 09:01
@metcoder95
Copy link
Member

cc: @flakey5

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I have absolutely no clue, because that require/import always throw that error code from Node.js core. Something, something is 100% off on your end due to some monkeypatching one of your dependencies is making.

I recommend we drop the if-check completely.

@metcoder95
Copy link
Member

@xconverge you mentioned PM2, I'd advice opening at their project

@xconverge
Copy link
Contributor Author

@mcollina in the stack trace for the issue you can see that https://www.npmjs.com/package/require-in-the-middle is used by pm2, so yes there is definitely some monkey patching going on. I agree the if check should be dropped completely.

@metcoder95 see my message above, I do realize that this is related to pm2 specifically, but I don't think undici should be failing catastropically here for an optional/experimental feature check when we could just skip it and keep going. I am not even sure I agree that an issue needs to be opened with pm2 and/or require-in-the-middle, I think that this check should be less strict for greater compatibility

@xconverge xconverge requested a review from mcollina November 29, 2024 16:15
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 64799e6 into nodejs:main Dec 1, 2024
30 of 31 checks passed
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
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.

V7 unexpected sqlite error when using PM2
4 participants