kubernetes-sigs / kui

A hybrid command-line/UI development experience for cloud-native development
Apache License 2.0
2.81k stars 183 forks source link

feat: remove need for babel (except for mocha tests) #9337

Closed starpit closed 1 year ago

starpit commented 1 year ago

Description of what you did:

This updates the way we prescan the plugins so as to remove any need for CJS modules. We can thus remove one of the last remaining uses of babel. All that remains is the mocha tests. Porting these will be a huge pain:

1) ts-node/esm+ts-node/register fail because of our use of a function(this: Common.ISuite). it fails to recognize our override of mocha's Suite type. admittedly this is probably just bad code on our part. if we wanted to use ts-node, we'd have to update every single test file to ... somehow do this differently 2) mocha's native support for ESM assumes either that the generated ESM files are named .mjs (which is not possible in typescript https://github.com/microsoft/TypeScript/issues/18442), or that the tests are in a "type": "module" module (which would require substantial changes to our entire code base) 3) port to ... playwright? which would also get us off the spectron hack branch

so... we are left with babel for the tests at the moment; the first option is the most attractive in terms of expediency (at least.. this would only affect the test files), while the second is what is really needed to make Kui ESM-native.

In summary: with this change, the kui build no longer generates any CJS modules (unless you use @kui-shell/test, and then only when you run the tests), but does not yet mandate the use of ESM from clients (because it is not yet ESM-native).

What is missing to be fully ESM? all imports need to specify the .js file, e.g. import('./foo/index.js'); we cannot use require.resolve; we'd have to find and fix any CJS-only npms (e.g. fs-extra pre v11)...

BREAKING CHANGE: this is a substantial change, and may introduce issues with plugins. It is unlikely, by it seems prudent to mark this as a major change.

Required Items

Optional Items

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: starpit

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](https://github.com/kubernetes-sigs/kui/blob/master/OWNERS)~~ [starpit] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment