chore(compass-web): build devtools-connect into compass web COMPASS-9793 #7588

nbbeeken · 2025-11-20T21:36:36Z

Description

https://jira.mongodb.org/browse/COMPASS-9793

Using devtools connect so we can pull in fast fail logic and other benefits

mongodb-js/devtools-shared#592

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
If this change updates the UI, screenshots/videos are added and a design review is requested
If this change could impact the load on the MongoDB cluster, please describe the expected and worst case impact
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

nbbeeken · 2025-11-21T23:32:54Z

packages/compass-web/polyfills/net/index.ts

-  setTimeout(() => {
-    socket.connect(options);
-  });
+  socket.connect(options);


This was making an exception uncatchable by the try/catch wrapping connect(). The exception was momentary while I was debugging stuff so not a code path used in practice, but curious to know why we put this in a setTimeout?

nbbeeken · 2025-11-21T23:35:08Z

packages/compass-web/polyfills/@mongodb-js/devtools-connect/index.ts

-      },
-      destroy() {
-        return Promise.resolve();
+      // eslint-disable-next-line @typescript-eslint/require-await


What is the motive for preferring synchronous functions that return a Promise vs using the keyword async that produces the same thing more succinctly?

Happy to put this back to Promise.resove/reject

If you just return a promise then you don't need to add eslint exceptions that you added here and an explanation for why you break the rule (that is missing 🙂). Less unexplained lint exceptions in the code is better than more unexplained lint exceptions

Well I have to agree that it's pretty annoying that eslint doesn't understand that "This function is async because not returning a Promise would break the API contract" even when it could infer that through types

It should in theory based on the docs, we can look more into why it doesn't work for us. Here specifically there are also no types for it to understand that there's an expected promise return

I will still say though that personally I find that at a glance explicit promise return seems clearer than async funciton with no await inside. Disabling eslint rules is also absolutely fine, but annotation should always be provided to make it clear why it's happening, similar the to rule we have for ts-expect-error

This function is async because not returning a Promise would break the API contract

Basically this trap is a little too easy to fall into esp with functions that are just for throwing. The systemCA function in the other file was synchronously throwing its real counter part is an async function.

In this case, making this misstep is more about missing TS, but generally throwing the keyword around is a way to not think about what you do inside the scope of the function but always get back the return type you want (a promise).

Anyway with the refactors made I think I removed any usage of this eslint exception. Fun fact async () => Promise.resolve(...) doesn't yell at you to await something 😅 best of both worlds??

packages/compass-web/polyfills/@mongodb-js/devtools-connect/index.ts

nbbeeken · 2025-11-21T23:36:41Z

packages/compass-web/polyfills/@mongodb-js/devtools-proxy-support/index.ts

+  const error = new Error('system CA access not available in compass-web');
+  (error as unknown as { code: 'OUT_OF_MEM' }).code = 'OUT_OF_MEM'; // This is a "tls" error code that makes devtools-connect not use the systemCA


This is a bit hacky, but arguably quite stable. Alternatively we can make useSystemCA configurable from the outside.

If we're planning on using all of the devtools-connect directly, we should make devtools-connect configurable enough for us to deal with these things without relying on some internal logic of it. I don't think it's a good idea to rely on the error code to force devtools-connect to try connection flow twice

packages/compass-web/polyfills/@mongodb-js/devtools-proxy-support/index.ts

nbbeeken · 2025-11-21T23:39:58Z

packages/compass-web/polyfills/net/index.ts

+    // WS does not support callback lookup
+    if ((lookup?.length ?? 0) > 0) lookup = undefined;


It is a little inconvenient that this synchronous for CompassWeb and async everywhere else even if it is returning a dummy value. Should we refactor to return the cluster info in a callback?

We are deliberatly not defining this function in a way that would mess up with the original interface as in theory it should be still passed down and be callable by the internals that we don't control and behave in a way that is matching the net interface

That totally makes sense. Should it throw then if it's given the interface it doesn't support? or more accurately invoke the given callback with an error? any callers will probably hang if we don't do something with it.

Good point, we should probably make the implementation in the connection storage for web fully matching the interface and call a callback with an error

gribnoysup · 2025-11-24T13:04:52Z

packages/compass-web/polyfills/@mongodb-js/devtools-connect/index.ts

If we're calling the original package directly and still have to stub a bunch of its dependencies for it not to blow up in web, why do we need this local polyfill still?

I think I thought I needed the indirection still to fix the systemCA thing but if we merge the new option then we should be good once that's passing through.

gribnoysup · 2025-11-24T13:06:53Z

packages/compass-web/polyfills/@mongodb-js/devtools-connect/index.ts

+import * as devtools_connect from 'devtools-connect-original';
+import { createMongoDBOIDCPlugin } from '../oidc-plugin';


Related to https://github.com/mongodb-js/compass/pull/7588/files#r2556225208: this seems redundant, devtools connect will import the polyfill that you provided

addaleax · 2025-11-24T13:26:41Z

packages/compass-web/polyfills/@mongodb-js/devtools-connect/index.ts

-        return Promise.resolve();
+      // eslint-disable-next-line @typescript-eslint/require-await
+      async getStateShareServer() {
+        return 'Not Available';


I know I wrote this code, but maybe we can throw here or return e.g. null instead? Or otherwise add a comment that this happens to be a return value that "properly" indicates the N/A state because it happens to not be valid JSON? (esp. because the fact that this value needs to be JSON in order to be valid is an implementation detail of devtools-connect)

nbbeeken force-pushed the COMPASS-9793 branch 2 times, most recently from f5b05e5 to 823c330 Compare November 21, 2025 21:30

nbbeeken commented Nov 21, 2025

View reviewed changes

nbbeeken force-pushed the COMPASS-9793 branch from 823c330 to 8db4753 Compare November 22, 2025 01:36

chore(compass-web): build devtools-connect into compass web COMPASS-9793

79d4fec

nbbeeken force-pushed the COMPASS-9793 branch from 8db4753 to 79d4fec Compare November 22, 2025 01:37

gribnoysup reviewed Nov 24, 2025

View reviewed changes

addaleax reviewed Nov 24, 2025

View reviewed changes

removing devtools-connect indirection

74a1476

nbbeeken mentioned this pull request Nov 26, 2025

feat(devtools-connect): fail fast on specific error and codes from compass-web and add useSystemCA to public options COMPASS-9793 mongodb-js/devtools-shared#592

Open

3 tasks

		const error = new Error('system CA access not available in compass-web');
		(error as unknown as { code: 'OUT_OF_MEM' }).code = 'OUT_OF_MEM'; // This is a "tls" error code that makes devtools-connect not use the systemCA

		// WS does not support callback lookup
		if ((lookup?.length ?? 0) > 0) lookup = undefined;

		import * as devtools_connect from 'devtools-connect-original';
		import { createMongoDBOIDCPlugin } from '../oidc-plugin';

chore(compass-web): build devtools-connect into compass web COMPASS-9793 #7588

Are you sure you want to change the base?

chore(compass-web): build devtools-connect into compass web COMPASS-9793 #7588

Uh oh!

Conversation

nbbeeken commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nbbeeken commented Nov 20, 2025 •

edited

Loading