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

JSX types are augmented incorrectly when importing an instantsearch package #5853

Closed
1 task done
sachinahya opened this issue Sep 18, 2023 · 7 comments · Fixed by #5882
Closed
1 task done

JSX types are augmented incorrectly when importing an instantsearch package #5853

sachinahya opened this issue Sep 18, 2023 · 7 comments · Fixed by #5882
Labels
triage Issues to be categorized by the team Type: Bug

Comments

@sachinahya
Copy link

🐛 Current behavior

Importing the package seems to import globally augmented versions of various types in the JSX namespace. This has the side effect of clashing and being incorrect against the versions in @types/react and @types/react-dom.

This seems to have been caused by algolia/ui-components#16 which declares the augmentations inside a global block, affecting anything that imports these packages.

🔍 Steps to reproduce

An example is included in the CodeSandbox below - see src/App.tsx.

The Name component has been supplied with a ref prop, which should be a TypeScript error because Name does not list that props in its type. However, due to the global augmentation of InstristicAttributes, this is not an error and the @ts-expect-error comment is incorrectly reported as unused.

Commenting out the import for react-instantsearch correctly reports an error for the ref prop not existing on the component.

Live reproduction

https://codesandbox.io/p/sandbox/algolia-ts-issue-43d7hp?file=/src/App.tsx

💭 Expected behavior

Importing any of the instantsearch or ui-components packages should not be incorrectly augmenting the types for the project they are imported into. Specifically in this case, they should not be adding a ref prop to all JSX elements.

It now means that all of our components can appear to take a ref according to their types, leading to confusion as to which components actually do take a ref at runtime.

I believe the solution here is to declare the JSX types needed inside a .d.ts file within the repository and include it as a reference in the tsconfig.json, so that it can exist only at build time for compilation and not at runtime where it interferes with existing declarations.

Package version

react-instantsearch 7.0.3

Operating system

macOS 13.4.1

Browser

Chrome 117.0.5938.88

Code of Conduct

  • I agree to follow this project's Code of Conduct
@sachinahya sachinahya added the triage Issues to be categorized by the team label Sep 18, 2023
@JClackett
Copy link

Yesss, I also have an issue where algolia is overiding the global jsx key type:

declare global {
    namespace JSX {
        interface IntrinsicAttributes {
            key?: string | number | null;
            ref?: unknown;
        }
        interface ElementChildrenAttribute {
            children: ComponentChildren;
        }
        interface Element extends VNode {
        }
        interface IntrinsicElements {
        }
    }
}

however react allows setting a bigint as a type so it fails everywhere in my app..

 Types of property 'key' are incompatible.
      Type 'Key | null | undefined' is not assignable to type 'string | number | null | undefined'.
        Type 'bigint' is not assignable to type 'string | number | null | undefined'.ts(2322)

@thejessewinton
Copy link

+1, bumping this, currently blocking me from being able to ship!

@Haroenv
Copy link
Contributor

Haroenv commented Oct 4, 2023

We think we have found a solution for now, if you change the built version of ui-components to be the one from this PR: algolia/ui-components#29, does it work for you all @thejessewinton, @JClackett, @sachinahya?

@thejessewinton
Copy link

thejessewinton commented Oct 4, 2023

@Haroenv thanks, I will give it a try. When do you think this will be released?

@Haroenv
Copy link
Contributor

Haroenv commented Oct 4, 2023

Once it's confirmed this works as expected in some diverse environments @thejessewinton :)

@sachinahya
Copy link
Author

@Haroenv Thanks for doing this. I built and npm linked this locally and it seems to have solved our issue.

@thejessewinton
Copy link

@Haroenv same result as @sachinahya, thanks for your work on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Issues to be categorized by the team Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants