fix(tscjs): fix typescript in cjs builds by using a separate type import #2935
Conversation
🦋 Changeset detectedLatest commit: 009982b The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
@djMax is attempting to deploy a commit to the Ariakit Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 009982b:
|
|
Hi, @djMax. Thanks for the PR! Could you please create a reproduction of the issue this PR resolves? |
scripts/build/utils.js
Outdated
| if (includeTypes) { | ||
| return { | ||
| import: { | ||
| default: `./${join(esmDir, path)}.js`, | ||
| types: `./${join(cjsDir, path)}.d.ts`, | ||
| }, | ||
| require: { | ||
| default: `./${join(cjsDir, path)}.cjs`, | ||
| types: `./${join(cjsDir, path)}.d.cts`, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
According to the TypeScript documentation, the types field should be listed first. However, the documentation also notes that it will search for a declaration file in the same location. So, I'm curious whether it's necessary to explicitly set it here.
There was a problem hiding this comment.
It is. But I could certainly flip the order. In terms of repro, should I do that just as a separate git repo or is there a more typical place to do that in ariakit? i.e. do we use stackblitz or some similar thing.
There was a problem hiding this comment.
It is.
Is it still necessary if you remove the .d.ts files from the folder, only leaving the .d.cts ones?
There was a problem hiding this comment.
If we only had the .d.cts, then we wouldn't be pointing to in properly in the global "types" key. All of this is premised on the idea that upstream users of ariakit should not be forced into type: module, as evidenced by the multiple build outputs and the multiple import paths. This PR just fixes that to include Typescript situations that do not rely on ESM.
|
Thanks! I couldn't access that page, but I could see the source code when I changed the URL to https://stackblitz.com/edit/stackblitz-starters-j4p32y. Looks like there's no |
|
Because, like many people, we rely on things that cannot handle type: module just yet. ESM is a s**t show, we don't want ariakit being the thing forcing people to endure the pain (IMHO). |
|
I understand not every project can be ESM right now. And Ariakit doesn't force this option. My question was about using |
|
The short answer is I'm not sure. The longer one is that I think all modern large projects seem to balance on the head of a pin with ESM: between Next.js, Typescript, Vitest, Jest, sindresorhus's refusal to do anything except ESM, webpack, Storybook, etc. So it is a supported setting (NodeNext for module/moduleResolution) and working for us everywhere, EXCEPT that for some reason Typescript gets confused about the exports if there is no separate type detail in the export key in package.json. I cannot begin to fathom all the intricacies of why or why our pin is not currently poking us in the eye. |
There was a problem hiding this comment.
I've updated the build script to copy all CJS declaration files to their corresponding .d.cts versions, so it also works with deep path imports. I tested locally with various combinations of type, module, and moduleResolution values, and everything appears to be functioning well.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @ariakit/core@0.3.4 ### Patch Changes - [`#2935`](#2935) Fixed TypeScript declaration files in CommonJS projects using `NodeNext` for `moduleResolution`. - [`#2945`](#2945) Added new `disabledFromProps` function to `@ariakit/core/utils/misc`. - [`#2948`](#2948) Added `"use client"` directive to all modules. - Improved JSDocs. ## @ariakit/react@0.3.5 ### Patch Changes - [`#2935`](#2935) Fixed TypeScript declaration files in CommonJS projects using `NodeNext` for `moduleResolution`. - [`#2945`](#2945) Added `name` and `value` properties to non-native input elements rendered by [`Checkbox`](https://ariakit.org/reference/checkbox), [`Radio`](https://ariakit.org), [`MenuItemCheckbox`](https://ariakit.org/reference/menu-item-checkbox), and [`MenuItemRadio`](https://ariakit.org/reference/menu-item-radio). It's now possible to access the `name` and `value` properties from the `event.target` element in the [`onChange`](https://ariakit.org/reference/checkbox#onchange) event handler. - [`#2945`](#2945) Fixed [`CompositeItem`](https://ariakit.org/reference/composite-item) and associated components not receiving the [`disabled`](https://ariakit.org/reference/composite-item#disabled) prop when it's being used by a higher-level component such as [`MenuItemCheckbox`](https://ariakit.org/reference/menu-item-checkbox) or [`MenuItemRadio`](https://ariakit.org/reference/menu-item-radio). - [`#2945`](#2945) It's now possible to control the menu [`values`](https://ariakit.org/reference/menu-provider#values) state by passing the [`checked`](https://ariakit.org/reference/menu-item-checkbox#checked), [`defaultChecked`](https://ariakit.org/reference/menu-item-checkbox#defaultchecked) and [`onChange`](https://ariakit.org/reference/menu-item-checkbox#onchange) props to [`MenuItemCheckbox`](https://ariakit.org/reference/menu-item-checkbox) and [`MenuItemRadio`](https://ariakit.org/reference/menu-item-radio). - [`#2948`](#2948) Added `"use client"` directive to all modules. - [`#2949`](#2949) The [Select](https://ariakit.org/components/select) component will now display the selected option(s) on the underlying native select element even when the corresponding [`SelectItem`](https://ariakit.org/reference/select-item) components aren't rendered. This comes in handy when the [`SelectPopover`](https://ariakit.org/reference/select-popover) component is rendered dynamically (for instance, using the [`unmountOnHide`](https://ariakit.org/reference/select-popover#unmountonhide) prop) or a [`defaultValue`](https://ariakit.org/reference/select-provider#defaultvalue) is given without a matching [`SelectItem`](https://ariakit.org/reference/select-item) component. - Improved JSDocs. - Updated dependencies: `@ariakit/react-core@0.3.5`. ## @ariakit/react-core@0.3.5 ### Patch Changes - [`#2935`](#2935) Fixed TypeScript declaration files in CommonJS projects using `NodeNext` for `moduleResolution`. - [`#2945`](#2945) Added `name` and `value` properties to non-native input elements rendered by [`Checkbox`](https://ariakit.org/reference/checkbox), [`Radio`](https://ariakit.org), [`MenuItemCheckbox`](https://ariakit.org/reference/menu-item-checkbox), and [`MenuItemRadio`](https://ariakit.org/reference/menu-item-radio). It's now possible to access the `name` and `value` properties from the `event.target` element in the [`onChange`](https://ariakit.org/reference/checkbox#onchange) event handler. - [`#2945`](#2945) Fixed [`CompositeItem`](https://ariakit.org/reference/composite-item) and associated components not receiving the [`disabled`](https://ariakit.org/reference/composite-item#disabled) prop when it's being used by a higher-level component such as [`MenuItemCheckbox`](https://ariakit.org/reference/menu-item-checkbox) or [`MenuItemRadio`](https://ariakit.org/reference/menu-item-radio). - [`#2945`](#2945) It's now possible to control the menu [`values`](https://ariakit.org/reference/menu-provider#values) state by passing the [`checked`](https://ariakit.org/reference/menu-item-checkbox#checked), [`defaultChecked`](https://ariakit.org/reference/menu-item-checkbox#defaultchecked) and [`onChange`](https://ariakit.org/reference/menu-item-checkbox#onchange) props to [`MenuItemCheckbox`](https://ariakit.org/reference/menu-item-checkbox) and [`MenuItemRadio`](https://ariakit.org/reference/menu-item-radio). - [`#2948`](#2948) Added `"use client"` directive to all modules. - [`#2949`](#2949) The [Select](https://ariakit.org/components/select) component will now display the selected option(s) on the underlying native select element even when the corresponding [`SelectItem`](https://ariakit.org/reference/select-item) components aren't rendered. This comes in handy when the [`SelectPopover`](https://ariakit.org/reference/select-popover) component is rendered dynamically (for instance, using the [`unmountOnHide`](https://ariakit.org/reference/select-popover#unmountonhide) prop) or a [`defaultValue`](https://ariakit.org/reference/select-provider#defaultvalue) is given without a matching [`SelectItem`](https://ariakit.org/reference/select-item) component. - Improved JSDocs. - Updated dependencies: `@ariakit/core@0.3.4`. ## @ariakit/test@0.3.1 ### Patch Changes - [`#2935`](#2935) Fixed TypeScript declaration files in CommonJS projects using `NodeNext` for `moduleResolution`. - [`#2948`](#2948) Added `"use client"` directive to all modules. - Updated dependencies: `@ariakit/core@0.3.4`. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
Excellent, that is definitely better, so I appreciate it! |
Closes #2356
Taking this PR from another project as inspiration:
openapi-ts/openapi-typescript#1360
This should fix ariakit-react typing in Typescript projects using NodeNext module/moduleresolution by explicitly pointing at a "cts" type definition file. It should not bother existing use cases but I left index.d.ts in the cjs module just in case.