Skip to content

Update openid-client to v6#1394

Open
alxndrsn wants to merge 32 commits into
getodk:masterfrom
alxndrsn:openid-6
Open

Update openid-client to v6#1394
alxndrsn wants to merge 32 commits into
getodk:masterfrom
alxndrsn:openid-6

Conversation

@alxndrsn

@alxndrsn alxndrsn commented Feb 10, 2025

Copy link
Copy Markdown
Contributor

openid-client v6 is a complete rewrite, with API changes: https://github.com/panva/openid-client/releases/tag/v6.0.0

API reference: https://github.com/panva/openid-client/blob/v6.1.7/docs/README.md

Closes getodk/central#1225

What has been done to verify that this works as intended?

Existing tests.

Why is this the best possible solution? Were any other approaches considered?

Staying up-to-date with security-focussed libraries seems like a sensible approach. An alternative might be to fork the openid-client library, but this seems like high-risk, unnecessary work for no obvious benefit.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It should not affect users.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

No.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

v6 is a complete rewrite, with API changes: https://github.com/panva/openid-client/releases/tag/v6.0.0

Closes #1230
Comment thread test/integration/api/oidc.js
Comment thread test/integration/api/oidc.js Outdated
Comment thread lib/util/oidc.js
Comment thread lib/resources/oidc.js Outdated
Comment thread lib/resources/oidc.js Outdated
Comment thread lib/resources/oidc.js
@alxndrsn

This comment was marked as resolved.

@alxndrsn

This comment was marked as resolved.

@alxndrsn alxndrsn marked this pull request as ready for review February 11, 2025 14:03
Comment thread test/integration/api/oidc.js
@alxndrsn alxndrsn requested a review from sadiqkhoja March 13, 2025 16:16

@sadiqkhoja sadiqkhoja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Nothing to change here but I'd like mention that I am getting Support for loading ES Module in require() is an experimental feature and might change at any time warning. Maybe this feature will get matured or we may consider migrating to ESM 🤷‍♂️

Comment thread lib/resources/oidc.js
@alxndrsn

Copy link
Copy Markdown
Contributor Author

I am getting Support for loading ES Module in require() is an experimental feature and might change at any time warning.

Good spot - I'll take a look into this.

Maybe this feature will get matured or we may consider migrating to ESM 🤷‍♂️

I think we should avoid that migration as long as possible.

@alxndrsn

Copy link
Copy Markdown
Contributor Author

Support for loading ES Module in require() is an experimental feature and might change at any time

Current status:

Stability: 1.2 - Release candidate
https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require

which means

1.2 - Release candidate. Experimental features at this stage are hopefully ready to become stable. No further breaking changes are anticipated but may still occur in response to user feedback. We encourage user testing and feedback so that we can know that this feature is ready to be marked as stable.
https://nodejs.org/api/documentation.html#stability-index

I'll hold of merging for now.

@alxndrsn

alxndrsn commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Stability: 1.2 - Release candidate

Current NodeJS version for this repo: 24.14.1

Since 24.15.0, "Loading ECMAScript modules using require()" is no longer considered experimental.

It will be reasonable to consider this PR for merging very soon.

@lognaturel

Copy link
Copy Markdown
Member

@alxndrsn You're planning on merging this one for the release, right? Are you waiting until the node version upgrade to 24.17.0 is in? Seems to me like it would be ok to merge this one first and helpful to have it on the staging server for a little bit before regression testing.

@alxndrsn alxndrsn requested a review from lognaturel June 22, 2026 19:28
@alxndrsn

Copy link
Copy Markdown
Contributor Author

You're planning on merging this one for the release, right?

I hadn't been planning specifically, just waiting for the required feature to be fully supported by NodeJS. I don't recall a specific need to update to openid-client v6.

@lognaturel

Copy link
Copy Markdown
Member

I don't think there's anything specific prompting the upgrade other than wanting to stay up-to-date. If you don't think it's a must-have for this release, then how about we merge it at the beginning of the next release cycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openid-client: update to v6?

3 participants