Skip to content

Conversation

@kanadgupta
Copy link
Contributor

@kanadgupta kanadgupta commented Oct 31, 2023

🧰 Changes

Quick overview of what this PR does:

  • Adds the "type": "module" field to the package.json file in the codgen'd SDKs
  • Updates our exports entrypoints in the codegen'd SDKs accordingly
  • Adds .js file extensions to all of the relative file imports in the codegen'd TS files

I opened this PR in hopes of fixing the typing issues in RM-8230 (which I later determined were an issue with my test environment and not with these SDKs)... but I still think our codegen'd SDKs should incorporate these changes! Here's why:

  • ESM is the future and we should probably be using that as the default
  • Since we're already requiring modern Node/TS anyways, this has zero impact for CommonJS users
  • The .js convention for relative file imports feels safer and more future-proof, even if tsup is proactively handling that for us
  • With the --experimental-detect-module flag that has arrived in Node v21, we should be specifying the package.json type value anyways and it feels dated to specify commonjs there

🧬 QA & Testing

Do tests still pass?

@kanadgupta kanadgupta changed the title feat: convert codegen'd SDKs to module feat: convert SDK output to module Oct 31, 2023
@kanadgupta kanadgupta marked this pull request as ready for review November 1, 2023 20:58
@kanadgupta kanadgupta requested a review from erunion November 1, 2023 20:59
@kanadgupta kanadgupta added enhancement New feature or request area:api Issues related to the `api` CLI, which builds the SDKs labels Nov 1, 2023
@erunion erunion merged commit 0dfd14c into main Nov 2, 2023
@erunion erunion deleted the convert-codegend-SDK-to-module branch November 2, 2023 15:42
@erunion erunion added this to the v7 milestone Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:api Issues related to the `api` CLI, which builds the SDKs enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants