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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to 0.22.2 fails due to use of new major version of client-node. #2014

Open
TonyJDavies opened this issue Nov 4, 2024 · 11 comments
Open

Comments

@TonyJDavies
Copy link

Describe the bug
When upgraded to latest version 0.22.2 of this library getting the following error:

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/app/node_modules/openid-client/build/index.js from /home/app/node_modules/@kubernetes/client-node/dist/oidc_auth.js not supported.
Instead change the require of index.js in /home/app/node_modules/@kubernetes/client-node/dist/oidc_auth.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/home/app/node_modules/@kubernetes/client-node/dist/oidc_auth.js:5:35)
    at /home/app/node_modules/@kubernetes/client-node/dist/oidc_auth_delayed.js:44:70
    at async DelayedOpenIDConnectAuth.applyAuthentication (/home/app/node_modules/@kubernetes/client-node/dist/oidc_auth_delayed.js:44:22)
    at async KubeConfig.apply(--- cut ---)
    at async KubeConfig.applyOptions (/home/app/node_modules/@kubernetes/client-node/dist/config.js:388:9)
    at async KubeConfig.applyToRequest (/home/app/node_modules/@kubernetes/client-node/dist/config.js:110:9)

This appears to be due to using the new version of client-node.
** Client Version **
0.22.2

** Server Version **
e.g. 1.19.1

To Reproduce
Steps to reproduce the behavior:

Expected behavior
A clear and concise description of what you expected to happen.

** Example Code**
Code snippet for what you are doing

Environment (please complete the following information):

  • OS: [e.g. Windows, Linux]
  • NodeJS Version [eg. 10]
  • Cloud runtime [e.g. Azure Functions, Lambda]

Additional context
Add any other context about the problem here.

@brendandburns
Copy link
Contributor

I'll try to repro this. I would have thought that the unit tests for this code would have failed though.

Do you have a snippet of code that reproduces this?

Also, what version of Node are you using?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 4, 2024

It looks like #1969 bumped the openid-client dependency from 5.7.0 to 6.1.3. Looking at the v6.0.0 release notes, that dependency switched to being an ESM module. Since the TypeScript code is transpiled to use require(), I'm guessing that explains the error.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 4, 2024

I would have thought that the unit tests for this code would have failed though.

Similar to what I mentioned in #1975 - it might be worth running the tests against the transpiled code.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 18, 2024

I'm curious what the maintainers would think about reverting #1969 on the v0.x release line, and using this as a reason to move the 1.x release line to ESM. I think that would cause the fewest future headaches.

@mstruebing
Copy link
Member

@brendandburns I think that would be a very valid breaking change.
what do you think?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 23, 2024

@brendandburns do you have any thoughts on #2014 (comment)?

@brendandburns
Copy link
Contributor

I'm ok with that.

cjihrig added a commit to cjihrig/javascript that referenced this issue Nov 26, 2024
This is a substantial change that moves the release-1.x branch from
CommonJS to ESM as discussed in
kubernetes-client#2014. Please
review/test carefully.

This commit does the following things:

- Updates the tsconfig to generate ESM code. The formatter also
  automatically formatted this file.
- Add `"type": "module"` to the `package.json` so that consumers
  treat this as ESM instead of CJS.
- Updates all of the imports to be valid ESM imports. ESM requires
  that file extensions are provided. TypeScript is smart enough to
  handle the `.js` extensions, but TypeScript does not support
  adding the extensions at build time. See
  microsoft/TypeScript#16577 for more
  details about this. The `tsc-esm-fix` utility on npm was used to
  automatically add the extensions. The code generator probably
  needs to add support for this too if it doesn't already.
- Fixup the `jsonpath` imports, as this module does not have a
  default ESM export. This just means adding `* as` to the
  existing imports.
- Remove the `AbortError` export originating from `node-fetch`.
  Apparently this is not actually there.

I was able to successfully import the transpiled code using
`import * as k8s from './dist/index.js';`. This should hopefully
unblock running tests against the transpiled code as well (it
currently does not work because the openid-client and chai
dependencies have both moved to ESM only).
cjihrig added a commit to cjihrig/javascript that referenced this issue Dec 4, 2024
This is a substantial change that moves the release-1.x branch from
CommonJS to ESM as discussed in
kubernetes-client#2014. Please
review/test carefully.

This commit does the following things:

- Updates the tsconfig to generate ESM code. The formatter also
  automatically formatted this file.
- Add `"type": "module"` to the `package.json` so that consumers
  treat this as ESM instead of CJS.
- Updates all of the imports to be valid ESM imports. ESM requires
  that file extensions are provided. TypeScript is smart enough to
  handle the `.js` extensions, but TypeScript does not support
  adding the extensions at build time. See
  microsoft/TypeScript#16577 for more
  details about this. The `tsc-esm-fix` utility on npm was used to
  automatically add the extensions. The code generator probably
  needs to add support for this too if it doesn't already.
- Fixup the `jsonpath` imports, as this module does not have a
  default ESM export. This just means adding `* as` to the
  existing imports.
- Remove the `AbortError` export originating from `node-fetch`.
  Apparently this is not actually there.

I was able to successfully import the transpiled code using
`import * as k8s from './dist/index.js';`. This should hopefully
unblock running tests against the transpiled code as well (it
currently does not work because the openid-client and chai
dependencies have both moved to ESM only).
cjihrig added a commit to cjihrig/javascript that referenced this issue Dec 5, 2024
This is a substantial change that moves the release-1.x branch from
CommonJS to ESM as discussed in
kubernetes-client#2014. Please
review/test carefully.

This commit does the following things:

- Updates the tsconfig to generate ESM code. The formatter also
  automatically formatted this file.
- Add `"type": "module"` to the `package.json` so that consumers
  treat this as ESM instead of CJS.
- Updates all of the imports to be valid ESM imports. ESM requires
  that file extensions are provided. TypeScript is smart enough to
  handle the `.js` extensions, but TypeScript does not support
  adding the extensions at build time. See
  microsoft/TypeScript#16577 for more
  details about this. The `tsc-esm-fix` utility on npm was used to
  automatically add the extensions. The code generator probably
  needs to add support for this too if it doesn't already.
- Fixup the `jsonpath` imports, as this module does not have a
  default ESM export. This just means adding `* as` to the
  existing imports.
- Remove the `AbortError` export originating from `node-fetch`.
  Apparently this is not actually there.

I was able to successfully import the transpiled code using
`import * as k8s from './dist/index.js';`. This should hopefully
unblock running tests against the transpiled code as well (it
currently does not work because the openid-client and chai
dependencies have both moved to ESM only).
@dominykas
Copy link
Contributor

dominykas commented Dec 5, 2024

Is the intent then to make the client ESM-only as of v1? That's a rather disruptive change if v0.x is going to be discontinued, i.e. it forces people to upgrade to Node.js 22 or to switch to ESM if they want to continue using this?

Additionally, in the light of #1693 (comment) (i.e. the final v1 potentially getting released in the coming week), is the intent to merge this before then or are we going to get a v2 shortly?

@mstruebing
Copy link
Member

I think we want to have the v1.x ESM to be future proof.
We could still release v0.x with the changed k8s API, however, I personally would like to stop that as early as possible as maintaining two main branches is a big burden.

You could still use the already released version, most of the time they are even compatible with newer k8s versions but it also depends a lot of which features you are using.

I think we should probably continue releasing 0.x like 2 versions to give people time to adapt to that change after promoting v1.x as the official release.

What do you think @brendandburns ?

@dominykas
Copy link
Contributor

Going to play the devil's advocate a little bit (I don't have a major problem with going ESM-only, assuming the require(ESM) support in Node.js v22+ is less experimental than the documentation implies1).

Aside from the API change (which is very nice!), one of the most attractive features of v1 is the switch to fetch. Having to ignore the various alerts about old vulnerabilities in request is something that is very annoying in the 0.x. The switch to ESM will significantly raise the barrier for switching - people will either have to upgrade to Node.js v22 (which they should, in the next 12-24 months anyways, but we all know how these things go) or they will have to rewrite their code in ESM (which is expensive and is very hard to justify in terms of cost vs benefit).

Is there any chance v1 could be released in dual-mode? I haven't done this, but there's plenty of packages in the ecosystem that release both - CJS and ESM inside? Or is that affected by dependencies?

Footnotes

  1. Prediction: removing the command line flag will make people start using it now, and that means the implementation will be effectively locked in - changing it even behind a major version would be very disruptive.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 5, 2024

As you mentioned, require(esm) is now available by default on Node v22 and v23. I recall seeing discussion about possibly unflagging it on v20 if things go well, but I'm having trouble finding the discussion right now. My experience has been that require(esm) works well, and since this package doesn't have top level await, it can be used here. If it does get backported to v20, and with v18 going EOL in April, I think that provides pretty good coverage. Of course, there is also dynamic import() for people who are unwilling to move to ESM.

Of course, all of this is also driven by dependencies that are increasingly moving to ESM. As someone who very strongly prefers CJS over ESM, even I have to say I just don't think it's worth the maintenance burden. Resisting ESM at this point is kind of like trying to kick water uphill 😄.

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

No branches or pull requests

5 participants