-
Notifications
You must be signed in to change notification settings - Fork 9
feat(devtools-connect): fail fast on specific error and codes from compass-web COMPASS-9793 #592
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
base: main
Are you sure you want to change the base?
Conversation
| "compilerOptions": { | ||
| "target": "es2020", | ||
| "lib": ["es2020", "DOM"], | ||
| "lib": ["es2021", "DOM"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMPASS_SOCKET_SERVICE
Are we renaming CCS? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the acronym CSS 🙂 /s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
( My not-so subtle move towards that direction........... 👀 ) happy to pick another name
Description
Added my new tls polyfill specific error and its codes to this fail fast detection layer.
https://github.com/10gen/mms/pull/147384
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