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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6702dc
fix: more specific exports
kanadgupta Sep 12, 2023
4d042f7
refactor: try module
kanadgupta Sep 12, 2023
2a4c3fb
chore: consolidate export declarations
kanadgupta Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

One more example, storybookjs/storybook#19269 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

And this tool can be used to check it https://github.com/arethetypeswrong/arethetypeswrong.github.io

Copy link
Member Author

@kanadgupta kanadgupta Sep 13, 2023 •
edited
Loading

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

"exports": {
".": {
"types": "./dist/index.d.ts",
"require": "./dist/index.js",
"import": "./dist/index.mjs"
"require": "./dist/index.cjs",
"import": "./dist/index.js"
},
"./analyzer": {
"types": "./dist/analyzer/index.d.ts",
"require": "./dist/analyzer/index.js",
"import": "./dist/analyzer/index.mjs"
"require": "./dist/analyzer/index.cjs",
"import": "./dist/analyzer/index.js"
},
"./lib/reducer": {
"types": "./dist/lib/reducer.d.ts",
"require": "./dist/lib/reducer.js",
"import": "./dist/lib/reducer.mjs"
"require": "./dist/lib/reducer.cjs",
"import": "./dist/lib/reducer.js"
},
"./rmoas.types": {
"types": "./dist/rmoas.types.d.ts",
"require": "./dist/rmoas.types.js",
"import": "./dist/rmoas.types.mjs"
"require": "./dist/rmoas.types.cjs",
"import": "./dist/rmoas.types.js"
},
"./operation": {
"types": "./dist/operation.d.ts",
"require": "./dist/operation.js",
"import": "./dist/operation.mjs"
"require": "./dist/operation.cjs",
"import": "./dist/operation.js"
},
"./operation/get-parameters-as-json-schema": {
"types": "./dist/operation/get-parameters-as-json-schema.d.ts",
"require": "./dist/operation/get-parameters-as-json-schema.js",
"import": "./dist/operation/get-parameters-as-json-schema.mjs"
"require": "./dist/operation/get-parameters-as-json-schema.cjs",
"import": "./dist/operation/get-parameters-as-json-schema.js"
},
"./package.json": "./package.json",
"./utils": {
"types": "./dist/utils.d.ts",
"require": "./dist/utils.js",
"import": "./dist/utils.mjs"
"require": "./dist/utils.cjs",
"import": "./dist/utils.js"
}
},
"main": "dist/index.js",
"module": "dist/index.mjs",
"types": "dist/index.d.ts",
"main": "dist/index.cjs",
"module": "dist/index.js",
"types": "dist/index.d.cts",
"engines": {
"node": ">=16"
},
Expand Down