feat(devtools-connect): make dns lookup configurable COMPASS-9793 #602
Conversation
| "check": "npm run typecheck && npm run lint && npm run depcheck", | ||
| "check-ci": "npm run check", | ||
| "testonly": "nyc mocha --colors -r ts-node/register src/**/*.spec.ts", | ||
| "testonly": "nyc mocha --colors --register ts-node/register src/**/*.spec.ts", |
There was a problem hiding this comment.
Assertions were not pointing to the correct line in the tests without this change and the debugger would not stop on the breakpoint lines. -r is mocha's "require", not sure if that's the same as node's 🤔
| # this is explicitly called as a dependency to make sure that is bootstrapped | ||
| # before mongodb-runner, since it is required via mongodb | ||
| - '@mongodb-js/saslprep' | ||
| - 'ts-node' # used by mocha |
There was a problem hiding this comment.
I guess this relates to the -r change, but not sure how.
| const customLookup = mongoClientOptions.lookup; | ||
| // Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458. | ||
| // Refs https://github.com/microsoft/vscode/issues/189805 | ||
| mongoClientOptions.lookup = (hostname, options, callback) => { | ||
| return dns.lookup(hostname, { verbatim: false, ...options }, callback); | ||
| return (customLookup ?? dns.lookup)( | ||
| hostname, | ||
| { verbatim: false, ...options }, | ||
| callback, | ||
| ); |
There was a problem hiding this comment.
| const customLookup = mongoClientOptions.lookup; | |
| // Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458. | |
| // Refs https://github.com/microsoft/vscode/issues/189805 | |
| mongoClientOptions.lookup = (hostname, options, callback) => { | |
| return dns.lookup(hostname, { verbatim: false, ...options }, callback); | |
| return (customLookup ?? dns.lookup)( | |
| hostname, | |
| { verbatim: false, ...options }, | |
| callback, | |
| ); | |
| // Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458. | |
| // Refs https://github.com/microsoft/vscode/issues/189805 | |
| mongoClientOptions.lookup ??= (hostname, options, callback) => { | |
| return dns.lookup(hostname, { verbatim: false, ...options }, callback); |
This could also just be ??= if we want to use whatever lookup we are given, but it seems more correct to me that this package owns its opinion on the verbatim flag
There was a problem hiding this comment.
Maybe also link to NODE-6437 given that passing the flag is a way to work around shortcomings of the driver, not something we should be doing here
| finalClientOptions.lookup('localhost', 27017, () => {}); // invoke it the way it would be by net/tls | ||
| expect(lookupSpy.getCalls()).to.have.lengthOf(1); // verify our input lookup was called instead of the dns package |
There was a problem hiding this comment.
bit of an odd test but since we're mocking things we don't actually get to the point where the driver would cause lookup to be invoked, by calling it here I'm checking that our input is wrapped rather than used directly.
nbbeeken
changed the title
nbbeeken
changed the title
|
coverage: 77.955% (+4.3%) from 73.608% |
Description
Continuation of the fail fast error changes. Our unconditional override of the lookup function makes CompassWeb's customization of lookup unusable by the socket layer.
Open Questions
Instead of respecting the custom option directly I was careful to still wrap the invocation in something that enforces verbatim to false. Do we prefer to just use lookup as is?
Checklist