Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update oidc-provider library to v8 #1662

Merged
merged 6 commits into from
Oct 6, 2023

Conversation

joachimvh
Copy link
Member

@joachimvh joachimvh commented Jun 16, 2023

馃搧 Related issues

Closes #1304

鉁嶏笍 Description

Updates the oidc-provider library to v8. There are several issues with this upgrade though, which require several hacky solutions to solve, so I'm not sure yet if this is the solution to go with and if perhaps some more of it can be automated.

The new version of oidc-provider is an ESM package. CSS is a CJS package though, and combining those two introduces several issues. Adding TypeScript and Jest to the mix complicates it even further. Some of the problems, not all though, could be avoided by making CSS an ESM package as well, but that would complicate things for people who use it as a dependency so I would prefer not to do that.

The issues are each described and resolved in the separate commits, but I'll also list them below.

1. ESM packages need to be dynamically imported in CJS

The only way to use an ESM package in CJS is by doing a dynamic import, see https://nodejs.org/api/esm.html#import-expressions. This means that to create the OIDC provider we now need the following line:

const provider = new (await import('oidc-provider')).default(this.baseUrl, config);

2. TypeScript settings need to change to support dynamic imports

By default TypeScript converts the dynamic imports to require statements, which would break the ESM import. To keep the dynamic import correct, moduleResolution needs to be set to node16. I don't think this has any consequences for packages depending on CSS so this should be fine.

Related issue: microsoft/TypeScript#43329

3. TypeScript has an issue with types from ESM packages

TypeScript also requires dynamic imports for all the typings of an ESM package. This would make the typings mostly unusable though. This is an open issue: microsoft/TypeScript#49721

To work around this, I copied the typing file to a local file and use that as typings for now, fooling TypeScript into thinking these are independent of the ESM package, and also showing there is no reason TypeScript shouldn't support this.

4. Jest support for ESM dependencies isn't perfect

There are several ways to work around the issues Jest has with such dependencies, but the easiest one seems to be to transform those packages into CJS before Jest interprets them. The jest config has been updated to use such a transformer, jest-esm-transformer-2, targetting a list of ESM dependencies. That list was found by starting from oidc-provider and always adding the next package that results in an error.

5. Jest has segmentation faults when using dynamic imports

This is a bug in Node.js with the components Jest uses: nodejs/node#35889.

To work around this, the dynamic import of oidc-provider in IdentityProviderFactory is handled differently when Jest is running. Environment variables are checked to see if the code is being run in Jest, if yes, a specific Jest import is used instead of a dynamic import, preventing the segmentation fault.

For the unit tests, a different tsconfig.json is used that does not have the new moduleResolution value to prevent Jest issues there.

6. There is a bug in the authn client

See inrupt/solid-client-authn-js#2985. This is nothing major as it does not require a major fix in the library, but does prevent the CSS tests from succeeding currently.

@elf-pavlik
Copy link
Contributor

could be avoided by making CSS an ESM package as well, but that would complicate things for people who use it as a dependency so I would prefer not to do that.

Lack of support for ESM in Component.js could also make it harder

Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

minor tweaks to comments for clarity

src/identity/configuration/IdentityProviderFactory.ts Outdated Show resolved Hide resolved
src/identity/configuration/IdentityProviderFactory.ts Outdated Show resolved Hide resolved
@elf-pavlik
Copy link
Contributor

elf-pavlik commented Sep 25, 2023

Is this PR blocked by inrupt/solid-client-authn-js#2985 ?

@joachimvh
Copy link
Member Author

Is this PR blocked by inrupt/solid-client-authn-js#2985 ?

Yes as long as that isn't fixed the authn library won't work with the new version. The browser version of the library will work as the issue was fixed there in the past, the problem is with the node version of the library.

@joachimvh
Copy link
Member Author

I've added a workaround for the authn issue so the CSS integration tests can pass until the issue is fixed. So will look into merging this and the node v18 stuff soon. This will break client implementations that use the node.js version of the library until the issue is fixed, but a similar workaround could be used there. As far as I know most client applications use the browser version so should hopefully not have too much impact.

The new version is an ESM package,
so we need to do a dynamic import as our package is CJS.
To correctly transpile the dynamic import,
moduleResolution needs to be set to node16.
See microsoft/TypeScript#43329
Due to v8 of oidc-provider being ESM,
we can't use the typings directly because of a TS bug:
microsoft/TypeScript#49721.
This works around that.
To prevent issues with Jest and ESM
we transform all ESM dependencies to CJS while testing.
Dynamic imports cause segmentation faults with Jest:
nodejs/node#35889.
We work around this by handling imports in IdentityProviderFactory
differently when Jest is running.
For unit tests we use a different tsconfig
that transpiles dynamic imports differently,
as those are also used in AppRunner.
@joachimvh joachimvh merged commit 1e3684b into versions/next-major Oct 6, 2023
28 checks passed
@joachimvh joachimvh deleted the chore/oidc-provider branch October 6, 2023 06:39
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.

None yet

3 participants