Conversation
| "compilerOptions": { | ||
| "target": "es2020", | ||
| "lib": ["es2020", "DOM"], | ||
| "lib": ["es2021", "DOM"], |
There was a problem hiding this comment.
this is just cus AggregateError was missing in the test type check, I think this is also ok for the shipped code
| error.message, | ||
| ); | ||
| return new RegExp( | ||
| String.raw`\b(${NODE_SOCKET_NON_RETRY_CODES.join('|')})\b`, |
There was a problem hiding this comment.
def overkill for me to hoist the node error codes but figured the pattern matched my new front end code stuff... this stuff is very unchanging so we can go back to not changing it. 😅
|
This can be merged and upstreamed but it won't take effect until https://github.com/10gen/mms/pull/147384 is merged and deployed. |
There was a problem hiding this comment.
Looks good so far, but keep in mind that compass-web doesn't use devtools-connect because (at least so far) none of the logic here was relevant for it. It's still mostly true with the exception of this particular fast failure stuff. So, as the comment at the top there suggests, we might want either move this code to its own thing or just re-export the fast failure logic in a way that will allow compass-web to only import it and not everything else
| 'ENETUNREACH', | ||
| 'EINVAL', | ||
| ]; | ||
| const COMPASS_SOCKET_SERVICE_NON_RETRY_CODES = [ |
There was a problem hiding this comment.
COMPASS_SOCKET_SERVICE
Are we renaming CCS? 🙂
There was a problem hiding this comment.
We can use the acronym CSS 🙂 /s
There was a problem hiding this comment.
( My not-so subtle move towards that direction........... 👀 ) happy to pick another name
There was a problem hiding this comment.
Just worried that until we rename it, unfamiliar people might be confused what compass socket service refers to. Don't have a strong preference, but maybe at least worth dropping a comment here mentioning that this is cluster connection servcie
nbbeeken
force-pushed
the
COMPASS-9793
branch
from
00c4b29 to
f20b109
Compare
nbbeeken
force-pushed
the
COMPASS-9793
branch
from
f20b109 to
7c0317f
Compare
nbbeeken
changed the title
| MongoClientClass, | ||
| useSystemCA: clientOptions.useSystemCA ?? true, | ||
| }; | ||
| delete clientOptions.useSystemCA; |
There was a problem hiding this comment.
Nit: so that you don't need to delete it
| delete clientOptions.useSystemCA; | |
| { useSystemCA = true, ...clientOptions }: DevtoolsConnectOptions, | |
| logger: ConnectLogEmitter, | |
| MongoClientClass: typeof MongoClient, | |
| ): Promise<ConnectMongoClientResult> { | |
| detectAndLogMissingOptionalDependencies(logger); | |
| const options = { | |
| uri, | |
| clientOptions, | |
| logger, | |
| MongoClientClass, | |
| useSystemCA, | |
| }; |
There was a problem hiding this comment.
oh yea.. that's way better 😅
| 'ENETUNREACH', | ||
| 'EINVAL', | ||
| ]; | ||
| const COMPASS_SOCKET_SERVICE_NON_RETRY_CODES = [ |
There was a problem hiding this comment.
I think you might want to re-export these from here so that we can also use them in compass where we check that connection needs to be terminated after connection was successfully establised, specifically this code should probably be replaced by the import from here
3 participants
Description
Added my new tls polyfill specific error and its codes to this fail fast detection layer.
https://github.com/10gen/mms/pull/147384
mongodb-js/compass#7588
Also making useSystemCA publicly configurable here. For use in compass-web
Open Questions
Due to the layers of schtuff, is there a way I can npm link (not actually that) this over to compass then run compass sync with mms to try and manually test?
Checklist