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

fix: get exports working in TS nodeNext #796

Merged
merged 3 commits into from
Sep 12, 2023
Merged

fix: get exports working in TS nodeNext #796

merged 3 commits into from
Sep 12, 2023

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Sep 12, 2023

🧰 Changes

🧐 Background

When using the latest version of oas in readmeio/rdme#856, I'm seeing the following TypeScript errors:

CleanShot 2023-09-12 at 14 34 16@2x

This is happening because tsup generates two different type declaration files (one for CJS and one for ESM) that have slight differences...

Declaration files screenshot

CleanShot 2023-09-12 at 14 48 09@2x

... but the problem is that our package.json exports field only references the CJS type declaration files:

"types": "./dist/index.d.ts",

Per the TypeScript docs:

Attempting to use a single .d.ts file to type both an ES module entrypoint and a CommonJS entrypoint will cause TypeScript to think only one of those entrypoints exists, causing compiler errors for users of the package.

🌱 Solution

This PR makes two changes our package.json:

  • Dropped the types fields from the exports object. Turns out we don't have to specify the types fields at all since tsup names the declaration files properly so TypeScript automatically loads the co-located declaration files (source)
  • Flipped the package type over to a module and flipped the file extensions accordingly. This was required in order to get the sub-exports working properly in rdme (prettier@3 also is a module despite also having CJS exports) — the following weird ass error shows up in rdme if attempting to build and load in oas when it's not a module:
../oas/dist/operation-8f5d4769.d.ts:1:204 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("./rmoas.types.mjs")' call instead.

1 import { SchemaObject, OASDocument, OperationObject, HttpMethods, SecurityRequirementObject, KeyedSecuritySchemeObject, TagObject, ParameterObject, MediaTypeObject, ResponseObject, PathItemObject } from './rmoas.types.mjs';
                                                                                                                                                                                                             ~~~~~~~~~~~~~~~~~~~


Found 1 error in ../oas/dist/operation-8f5d4769.d.ts:1

📖 Further Reading

You can read about all of this in the TypeScript and Node docs:

https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing
https://nodejs.org/api/packages.html#nested-conditions

🧬 QA & Testing

If you check out this commit of rdme and npm link it to this branch of oas, you shouldn't see any build errors.

@@ -5,47 +5,41 @@
"license": "MIT",
"author": "ReadMe <support@readme.io> (https://readme.com)",
"sideEffects": false,
"type": "module",
Copy link
Member Author

@kanadgupta kanadgupta Sep 12, 2023

Choose a reason for hiding this comment

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

Now that this field is here, tsup automatically detects that and flips the extensions so now the dist/ directory looks like this:

.js / .d.ts = ESM
.cjs / .d.cts = CJS

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if im wrong but now the package is a full esm package

The correct way to fix it would be

       "./webpack": {
            "require": {
                "types": "./dist/webpack.d.ts",
                "default": "./dist/webpack.js"
            },
            "import": {
                "types": "./dist/webpack.d.mts",
                "default": "./dist/webpack.mjs"
            }
        },

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@kanadgupta kanadgupta Sep 13, 2023

Choose a reason for hiding this comment

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

@prisis that tool is fantastic, thanks for sharing!

The correct way to fix it

We don't need to explicitly define the type declaration files since TS will automatically pick up on the co-located files (see here for more info). This PR was published to oas@21.1 and it appears that the types are being picked up by that website properly:


Correct me if im wrong but now the package is a full esm package

Yeah if you read the PR description, we unfortunately needed to convert it over to ESM in order to get it working properly in another module: node16 + ESM repo I'm working on. If you look at the "before" results in the image below, it wasn't working properly in ESM nor in CJS.

The good thing is that the website says that we're still shipping our main CJS exports with the same warnings as we were before (see the "node16 from CJS" row on this page). Based on the warnings warnings, my guess is that these are issues with our tsup configuration or the code itself and not our package.json exports definition.

Before the exports fixes in this PR (oas@21.0.3):

CleanShot 2023-09-13 at 10 29 25@2x

After the exports fixes in this PR (oas@21.1.1):

CleanShot 2023-09-13 at 10 29 39@2x

@kanadgupta kanadgupta added the bug Something isn't working label Sep 12, 2023
@kanadgupta kanadgupta marked this pull request as ready for review September 12, 2023 20:28
@kanadgupta
Copy link
Member Author

@prisis FYI!

@erunion erunion merged commit 86d75f1 into main Sep 12, 2023
6 checks passed
@erunion erunion deleted the exports-fixes branch September 12, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

3 participants