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: TypeScript Type Portability Issues #4467

Merged
merged 127 commits into from
Jul 25, 2024

Conversation

aryaemami59
Copy link
Contributor

@aryaemami59 aryaemami59 commented Jun 19, 2024

Overview

The type portability issues in this repository are primarily caused by three specific TypeScript errors:

  1. TS4023: Exported variable 'useAppDispatch' has or is using name 'TimeResponse' from external module "/home/runner/work/redux-toolkit/redux-toolkit/examples/publish-ci/vite/src/app/services/times" but cannot be named.

    • Solution: This one can actually be resolved quite often by converting the problematic type from an interface to a type alias. Other times we might have to export the problematic type.
  2. TS2742: The inferred type of 'useGetQuotesQuery' cannot be named without a reference to '../../../node_modules/@reduxjs/toolkit/dist/query/react/buildHooks'. This is likely not portable. A type annotation is necessary.

    • Solution: Bundling the type definitions seems to have resolved this issue.
  3. TS2527: The inferred type of 'quotesApiSlice' references an inaccessible 'unique symbol' type. A type annotation is necessary.
    This one took a while for me to figure out, It turns out in order for a unique symbol type to be "accessible", the unique symbol has to be imported upon declaration, so we want something that looks like this:

    export declare const coreModuleName: unique symbol

    But dts-rollup-plugin gives us this instead:

    declare const coreModuleName: unique symbol
    export { coreModuleName }

    But TypeScript treats declare const coreModuleName: unique symbol and export { coreModuleName }; as 2 different unique symbols because each reference to coreModuleName is completely unique.

    • Solution: While somewhat hacky, I wrote a small script that takes all exported unique symbols and converts them from:

      declare const coreModuleName: unique symbol
      export { coreModuleName }

      To:

      export declare const coreModuleName: unique symbol

This PR:

  1. @examples-type-portability/bundler to test for "moduleResolution": "Bundler".
  2. @examples-type-portability/nodenext-cjs to test for "moduleResolution": "NodeNext" with TypeScript's CJS syntax and "type": "commonjs" in package.json.
  3. @examples-type-portability/nodenext-esm to test for "moduleResolution": "NodeNext" with ESM syntax and "type": "module" in package.json.

Things to keep in mind

  • Although I prefer using interfaces, it might be worthwhile to switch to type aliases going forward. interfaces are more prone to type portability issues compared to type aliases, which often dissolve into their constituents. I highly recommend using type aliases for all new types unless interfaces are necessary (e.g., for declaration merging).

  • I've noticed that using an interface as the return type of an external function often leads to type portability issues unless the interface itself is also exported.

- We also convert some of the problematic types from an `interface` to a `type` alias.
- The reason why `coreModuleName` and `reactHooksModuleName` are inaccessible is because in order for them to become public `unique symbols` we need to have `export declare const coreModuleName: unique symbol` as opposed to what `rollup-plugin-dts` does which is `declare const coreModuleName: unique symbol; export {
  coreModuleName }`.
Copy link

codesandbox bot commented Jun 19, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

codesandbox-ci bot commented Jun 19, 2024

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 bd83e94:

Sandbox Source
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration

Copy link

netlify bot commented Jun 19, 2024

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit bd83e94
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/66a19cb989b5cc000897560a
😎 Deploy Preview https://deploy-preview-4467--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@EskiMojo14
Copy link
Collaborator

this is going to cause a painful amount of conflicts with other PRs 😅

@aryaemami59
Copy link
Contributor Author

I know, I'm sorry :( but I really think this will be worth it.

@aryaemami59
Copy link
Contributor Author

Just out of curiosity what are some of the other PRs that are gonna have conflicts with this one? And is there anything I can do to minimize said conflicts?

@EskiMojo14
Copy link
Collaborator

the changes in this PR are very wide reaching (including some renames that seem unrelated to the issue at hand?), so i suspect quite a few will encounter issues, but in particular the creator PR #3837 is quite significant in how much it reworks, and will certainly encounter issues when trying to reconcile with this.

@aryaemami59
Copy link
Contributor Author

The renames were necessary, they were causing issues with name collisions, I'll see if I can work around it to minimize conflicts with other PRs.

@aryaemami59 aryaemami59 force-pushed the fix-type-portability-issues branch from 8490560 to c92ac90 Compare June 21, 2024 12:28
@aryaemami59 aryaemami59 force-pushed the fix-type-portability-issues branch 8 times, most recently from 2ca1c4a to da50c57 Compare July 9, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants