feat!: update openid-client to v6.x MONGOSH-2194 #218

addaleax · 2025-06-27T15:10:19Z

No description provided.

gagik · 2025-06-30T13:31:29Z

test/oidc-test-provider.ts

@@ -127,7 +127,7 @@ export class OIDCTestProvider {
    const { port } = this.httpServer.address() as AddressInfo;

    const app = express();
-    this.httpServer.on('request', app);


is this because it's an async callback now?

Yup! Just the linter complaining about the callback returning a Promise that is being ignored

gagik · 2025-06-30T13:45:54Z

src/util.ts

@@ -80,26 +84,33 @@ export function normalizeObject<T extends object>(obj: T): T {
  return Object.fromEntries(Object.entries(obj).sort()) as T;
 }

+function isURL(url: unknown): url is URL {
+  return Object.prototype.toString.call(url).toLowerCase() === '[object url]';


I'm curious, is there a reason why url instanceof URL would not work here?

Yeah, this is mostly me being generally mis-trusting of instanceof in JS since it won't work cross-context/cross-realm and there isn't usually a downside to using Object.prototype.toString. It's annoying if it works most of the time but then breaks down when you're trying to do something else with it

Honestly I'd love to avoid having this check here at all, but didn't find a good way to do that since the primary alternative is duck type testing, i.e. whether properties like href, hostname, etc. are present on it, and that felt a bit too permissive

@addaleax is this coming from your own experience or is there some reference of JS language features that wouldn't work as well across contexts? Is this about it being a later ES6 thing?

Yeah, this has definitely been an issue occasionally 🙂 It's the fact that instanceof performs equality comparisons along the prototype chain, but JS is not a language in which semantically identical classes actually compare equal.

Depending on the context you're working in, the two most common ways to run into this are a) having two different versions of a script in your source code, when the "same" class gets evaluated in each script it creates different instances that do not compare equal, or alternatively language or runtime built-ins from different realms (in JS specification language – this corresponds to vm.Context in Node.js or e.g. iframes in browsers).

So yeah, instanceof is often a code smell imo, it just checks the wrong thing.

gagik

Looks good to me, just another curiosity question

gagik · 2025-07-01T10:37:34Z

src/plugin.ts

+    try {
+      fetch = (await import('node-fetch')).default;
+    } catch {
+      fetch = await eval('import("node-fetch").default');


is this again from issues of across different contexts? I'm curious how an eval would work as a fallback

Ah yeah, this is something we already do in another place to ensure that this works both in webpack and in plain JS, with a bit more explanation:

oidc-plugin/src/plugin.ts

Lines 102 to 127 in 0bb5f2b

// 'open' 9.x+ is ESM-only. However, TypeScript transpiles

// the `await import()` here to `require()`, which fails to load

// the package at runtime. We cannot use one of the typical workarounds

// for loading ESM packages unconditionally, because we need to be

// able to webpack this file (e.g. in Compass), which means that we

// need to use imports with constant string literal arguments.

// eslint-disable-next-line @typescript-eslint/consistent-type-imports

let open: typeof import('open').default;

try {

open = (await import('open')).default;

} catch (err: unknown) {

if (

err &&

typeof err === 'object' &&

'code' in err &&

err.code === 'ERR_REQUIRE_ESM' &&

typeof __webpack_require__ === 'undefined'

) {

// This means that the import() above was transpiled to require()

// and that that require() called failed because it saw actual on-disk ESM.

// In this case, it should be safe to use eval'ed import().

open = (await eval("import('open')")).default;

} else {

throw err;

}

}

I'll add a comment to reference that section here

nirinchev

Nice work! I have fairly minor comments, none of which are blocking merging this as-is.

nirinchev · 2025-07-01T10:26:54Z

src/plugin.ts

+      headers: { ...options?.headers },
+      // TS is not convinced that node-fetch and built-in fetch are compatible enough
+    } as Parameters<typeof fetch>[1])) as unknown as Response;
+  };

  private async getOIDCClient(


[nit] should we rename this now that it's no longer returning a client?

nirinchev · 2025-07-01T10:54:37Z

src/util.ts

+      })();
+  }
+
+  serialize() {


[nit] should we explicitly specify the return type here? I know it gets inferred, but coming from strongly-typed languages, I tend to prefer more rigorous type definitions.

I very much tend to have the same preference 🙂

In this case, it's definitely not easy to specify the return type, both because it's about as complex as the function definition itself and because TS helpers like Omit do not play well with types like TokenEndpointResponse which contain both fixed properties with types and arbitrarily indexed keys. I'll still add a comment about this here

nirinchev · 2025-07-01T11:02:00Z

src/util.ts

+  }
+
+  serialize() {
+    const expiresIn: number | undefined = this.response.expiresIn();


[nit] we seem to be doing a whole lot of arithmetic converting between expires_in and expires_at and skimming through the usages of TokenSet, we don't ever seem to be using expiresIn() or the entire response for that matter. Would it make sense to make response private and instead expose getters for the fields/properties that are actually used? I know this is a bit inconsequential of a refactoring and we already have the code, so happy to leave things as they are, I just feel it might be a bit more obviouos to readers/consumers if we limited the API surface.

we don't ever seem to be using expiresIn()

We do, when serializing 🙂 We could avoid it by adding overloads to the constructor that handle both the "value comes from openid-client and only has .expiresIn()" and the "value comes from serialized storage and only has .expiresAt" cases, but that feels like overkill here

Would it make sense to make response private and instead expose getters for the fields/properties that are actually used?

I like this suggestion regardless though, so I just pushed a change accounting for this in the last commit 👍

feat!: update openid-client to v6.x MONGOSH-2194

4f2fec4

github-actions bot added breaking feature labels Jun 27, 2025

addaleax added 5 commits June 27, 2025 17:13

fixup: check

9ecfca3

fixup: bump mock provider package

9349a74

fixup: also run macOS azure tests in GHA

ed81b6e

fixup: also skip Azure on Evergreen macOS

89236ad

fixup: ready

f45a45c

addaleax marked this pull request as ready for review June 30, 2025 12:49

fixup: check

0cc4368

gagik reviewed Jun 30, 2025

View reviewed changes

gagik approved these changes Jul 1, 2025

View reviewed changes

nirinchev approved these changes Jul 1, 2025

View reviewed changes

fixup: review comments

fcbfaea

addaleax requested review from nirinchev and gagik July 2, 2025 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat!: update openid-client to v6.x MONGOSH-2194 #218

feat!: update openid-client to v6.x MONGOSH-2194 #218

addaleax commented Jun 27, 2025

Uh oh!

gagik Jun 30, 2025

Uh oh!

addaleax Jun 30, 2025

Uh oh!

gagik Jun 30, 2025 •

edited

Loading

Uh oh!

addaleax Jun 30, 2025

Uh oh!

gagik Jul 1, 2025 •

edited

Loading

Uh oh!

addaleax Jul 2, 2025

Uh oh!

gagik left a comment

Uh oh!

gagik Jul 1, 2025 •

edited

Loading

Uh oh!

addaleax Jul 2, 2025

Uh oh!

nirinchev left a comment

Uh oh!

nirinchev Jul 1, 2025

Uh oh!

nirinchev Jul 1, 2025

Uh oh!

addaleax Jul 2, 2025

Uh oh!

nirinchev Jul 1, 2025

Uh oh!

addaleax Jul 2, 2025

Uh oh!

Uh oh!

	// 'open' 9.x+ is ESM-only. However, TypeScript transpiles
	// the `await import()` here to `require()`, which fails to load
	// the package at runtime. We cannot use one of the typical workarounds
	// for loading ESM packages unconditionally, because we need to be
	// able to webpack this file (e.g. in Compass), which means that we
	// need to use imports with constant string literal arguments.
	// eslint-disable-next-line @typescript-eslint/consistent-type-imports
	let open: typeof import('open').default;
	try {
	open = (await import('open')).default;
	} catch (err: unknown) {
	if (
	err &&
	typeof err === 'object' &&
	'code' in err &&
	err.code === 'ERR_REQUIRE_ESM' &&
	typeof __webpack_require__ === 'undefined'
	) {
	// This means that the import() above was transpiled to require()
	// and that that require() called failed because it saw actual on-disk ESM.
	// In this case, it should be safe to use eval'ed import().
	open = (await eval("import('open')")).default;
	} else {
	throw err;
	}
	}

feat!: update openid-client to v6.x MONGOSH-2194 #218

Are you sure you want to change the base?

feat!: update openid-client to v6.x MONGOSH-2194 #218

Conversation

addaleax commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik left a comment

Choose a reason for hiding this comment

Uh oh!

gagik Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirinchev left a comment

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!

gagik Jun 30, 2025 •

edited

Loading

gagik Jul 1, 2025 •

edited

Loading

gagik Jul 1, 2025 •

edited

Loading