8000 fix(generators): Harden mongodb.js to reliably extract database from any connection string by jermsam · Pull Request #3264 · feathersjs/feathers · GitHub
[go: up one dir, main page]

Skip to content

fix(generators): Harden mongodb.js to reliably extract database from any connection string#3264

Merged
daffl merged 4 commits intofeathersjs:dovefrom
Jitpomi:dove
Jan 5, 2024
Merged

fix(generators): Harden mongodb.js to reliably extract database from any connection string#3264
daffl merged 4 commits intofeathersjs:dovefrom
Jitpomi:dove

Conversation

@jermsam
Copy link
Contributor
@jermsam jermsam commented Aug 21, 2023

The default connection for mongodb in mongodb.js fails when you try to connect with a connection string that uses mongodb-replicas sets or clusters ... (eg: mongodb://localhost:27017,127.0.0.1:27018/mo-db?replicaSet=rs0) ... This is because
new URL(connection).pathname.substring(1)
fails with an is not a valid URL error .... I propose the following replacement to mongodb.js as it will satisfy all most all formats of connection strings

@daffl
Copy link
Member
daffl commented Oct 11, 2023

Thank you for putting this PR up (and sorry for the late reply). I wonder if we should make this a helper since we'd want the generated code to be as short as possible.

@jermsam
Copy link
Contributor Author
jermsam commented Oct 13, 2023

Let me optimize it to be as short as possible

@jermsam
Copy link
Contributor Author
jermsam commented Oct 13, 2023

it's now just one additional line of code from what it was originally

@daffl daffl changed the title harden mongodb.js to reliably extract database from any connection st… fix(generators): Harden mongodb.js to reliably extract database from any connection string Jan 5, 2024
@daffl daffl merged commit 7b0f82c into feathersjs:dove Jan 5, 2024
@daffl
Copy link
Member
daffl commented Jan 5, 2024

Thank you for updating and sorry for the delay.

daffl added a commit that referenced this pull request Jan 5, 2024
… database from any connection string (#3264)"

This reverts commit 7b0f82c.
@daffl
Copy link
Member
daffl commented Jan 5, 2024

Apologies again but I had to revert the merge. I thought the tests were failing because things weren't up to date with latest but the error was actually an invalid regular expression that I wasn't able to fix (see errors in https://github.com/feathersjs/feathers/actions/runs/7427523402/job/20213354620#step:8:3307). You can run all test locally by running npm test in the main repo.

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

0