chore(service-provider-core): parse authMechProps rather than applying regex #1051
Conversation
addaleax
force-pushed
the
parse-authmechprops
branch
from
3e71b4f
to
6d9376e
Compare
|
This is failing CI right now because whatwg-url seems to be doing something weird in the browser-repl webpack variant – I’ll wait until #1055 is merged before looking into that, since this will need to be rebased anyway |
|
Also, I’ll need to update the mongodb-connection-string-url package anyway because it does case-sensitive matching on the properties (just like the previous existing code here) but it should be case-insensitive: https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst#naming |
…g regex This is a slightly cleaner way of accessing authMechanismProperties.
addaleax
force-pushed
the
parse-authmechprops
branch
from
6d9376e
to
9a215de
Compare
| @@ -6,7 +6,7 @@ module.exports = { | |||
| extensions: ['.tsx', '.ts', '.jsx', '.js', '.less'], | |||
| alias: { | |||
| // imports in service-provider-core that can break the browser build | |||
| 'whatwg-url': path.resolve(__dirname, 'empty.js'), | |||
| 'whatwg-url': path.resolve(__dirname, 'plain-url.js'), | |||
There was a problem hiding this comment.
@gribnoysup Does this seem okay to you?
There was a problem hiding this comment.
Yep, looks good to me! I'm wondering why is this required now for the browser-repl to work though 🤔 IIRC we had a workaround in service-provider-core that made the whole thing work regardless the environment it is used in and I assumed that this logic was moved to the mongodb-connection-string-url with other url parsing stuff
There was a problem hiding this comment.
Yeah – it turned out that Node.js and the browser actually have URL implementations that differ in some regards (mongodb-js/mongodb-connection-string-url#1), so I decided to just always go with whatwg-url and try to get consistent behavior.
There was a problem hiding this comment.
Okay, I should've searched better before asking, found my answer mongodb-js/mongodb-connection-string-url#1 😄 Okay, so based on that the only question I have is if this inconsistency between browser URL and whatwg-url library might be a problem for us when this alias makes browser-repl use browser URL instead of the library?
There was a problem hiding this comment.
... aaand I also don't refresh before posting, sorry 😓 Thanks for the providing the context, Anna!
There was a problem hiding this comment.
@gribnoysup Hm yeah … I wish I remembered what exactly caused problems here (aka I should have written that down in the PR description)
I think it was the fact that the driver also uses mongodb-connection-string-url now, and in Compass it picked up the browser URL, and the driver usage of it was causing trouble. But I guess you’re right, ideally we should be always using whatwg-url here as well … do you remember what exactly was breaking when we did that?
There was a problem hiding this comment.
Ohhh, I don't unfortunately, but I will try to reproduce/find out (should've also documented it that time it was happening 😅). Also if it's not failing immediately now and to not block this PR, I think it's okay to just merge it and look into it later, we would need to explicitly update it in compass anyway before this change lands in the app, so we will have a chance to catch issues that are not immediately obvious
This is a slightly cleaner way of accessing authMechanismProperties.