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

src: convert project from CJS to ESM #2062

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 26, 2024

This is a substantial change that moves the release-1.x branch from CommonJS to ESM as discussed in
#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 Provide a way to add the '.js' file extension to the end of module specifiers 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).

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 26, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 26, 2024
@brendandburns
Copy link
Contributor

Thanks for doing this. We do need to make the changes for upstream code in the upstream generator here:

https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/typescript-fetch

Because otherwise we'll overwrite these changes next time we regenerate.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 26, 2024

Oh, it looks like this might already be supported upstream: OpenAPITools/openapi-generator#15440

@brendandburns
Copy link
Contributor

Ah, cool, then we just need to update here:

https://github.com/kubernetes-client/gen/blob/master/openapi/typescript-fetch.xml#L26

To add that configuration.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 27, 2024

I ran the generator locally with the importFileExtension setting and then cherry-picked the resulting update on top of this branch. I encountered the following conflict:

diff --cc src/gen/http/http.ts
index 748aa4a2,cc9bdfbb..00000000
--- a/src/gen/http/http.ts
+++ b/src/gen/http/http.ts
@@@ -1,8 -1,8 +1,15 @@@
  // TODO: evaluate if we can easily get rid of this library
++<<<<<<< HEAD
 +import  FormData from "form-data";
 +import { URL, URLSearchParams } from 'url';
 +import * as http from 'http';
 +import * as https from 'https';
++=======
+ import  FormData from "form-data.js";
+ import { URL, URLSearchParams } from 'url.js';
+ import * as http from 'http.js';
+ import * as https from 'https.js';
++>>>>>>> 9f3cdd59... gen: generate with importFileExtension option
  import { Observable, from } from '../rxjsStub.js';
  
  export * from './isomorphic-fetch.js';

Looking at the generator source code, these lines appear to be incorrect and should not have the extension. So we will need an upstream patch after all - just a much smaller one.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 27, 2024

FWIW, I opened OpenAPITools/openapi-generator#20194. I have no idea what the PR process there is like, or how long it might be until it's available in this repo.

We could enable the importFileExtension now, and because the diff is so small, update the script in this repo to patch those four lines on update.

@brendandburns
Copy link
Contributor

They're usually pretty good about merging PRs fast. Let's give it a couple of days (maybe more because of the holiday) and then go the patch route if it isn't merged by sometime next week.

We're going to wait for Kubernetes 1.32 to release (12/11/24) before we cut the 1.0.0 anyway.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 3, 2024

The PR to the generator landed.

cjihrig@8973e71 is what updating OPENAPI_GENERATOR_COMMIT to OpenAPITools/openapi-generator@06f0b68 looks like for the v1.x branch, independent of anything related to this PR.

Is that something that should be PR'ed to this repo?

@mstruebing
Copy link
Member

Yes we would need this an then regenerate the code with the applied changes.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 3, 2024

OK. Here it is: #2085

cjihrig added a commit to cjihrig/gen that referenced this pull request Dec 3, 2024
This commit updates the TypeScript generator options to include
importFileExtension. This is done in order to support ESM in
the JavaScript client, as ESM requires explicit extensions.

Refs: kubernetes-client/javascript#2062
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 3, 2024

PR to update the XML config in the gen repo: kubernetes-client/gen#275

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2024
cjihrig added a commit to cjihrig/javascript that referenced this pull request Dec 3, 2024
k8s-ci-robot pushed a commit to kubernetes-client/gen that referenced this pull request Dec 3, 2024
This commit updates the TypeScript generator options to include
importFileExtension. This is done in order to support ESM in
the JavaScript client, as ESM requires explicit extensions.

Refs: kubernetes-client/javascript#2062
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 4, 2024
@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 4, 2024
@brendandburns
Copy link
Contributor

Looks like there are CI failures related to the script/test we use to make sure that package.json and package-lock.json are in-sync.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2024
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2024

Pushed up a fix for the script. The formatter did some extra work, but this is what I actually added:

import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);

@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label 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).
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2024
@mstruebing
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, cjihrig, mstruebing

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brendandburns,mstruebing]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 3acf719 into kubernetes-client:release-1.x Dec 5, 2024
8 checks passed
@cjihrig cjihrig deleted the esm branch December 5, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants