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

Expose ThreeElements interface #2347

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

AndrewPrifer
Copy link
Contributor

@AndrewPrifer AndrewPrifer commented Jul 1, 2022

This PR directly exposes all the r3f-specific React element types, instead of directly augmenting JSX.IntrinsicElements. JSX.IntrinsicElements still gets augmented, so it is fully backwards-compatible:

export interface ThreeElements {
  // ...
}

declare global {
  namespace JSX {
    interface IntrinsicElements extends ThreeElements {}
  }
}

This would be very useful for libraries that need to map over, or validate against r3f elements, like @theatre/r3f or @react-spring/three. Currently the only option is to use JSX.IntrinsicElements, which results in incorrect types that are way too broad and include other elements too, like those from react-dom. Furthermore since JSX.IntrinsicElements is huge, it can easily result in types that are either too complex for the type checker to handle, or too large for the compiler to serialize.

An interesting thing about the way this is solved in this PR, is that augmenting ThreeElements in turn will also augment JSX.IntrinsicElements:

declare module '@react-three/fiber' {
  interface ThreeElements {
    myMesh: Object3DNode<MyMesh, typeof MyMesh>
  }
}

The effect is that users would only have to augment a single interface, and it would automatically also work with all other libraries that depend on ThreeElements, while also exposing these elements in React.

@AndrewPrifer AndrewPrifer added enhancement New feature or request Typescript issues to do with TS labels Jul 1, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 1, 2022

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.

@AndrewPrifer AndrewPrifer self-assigned this Jul 1, 2022
@AndrewPrifer AndrewPrifer requested a review from gsimone July 1, 2022 11:12
@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Jul 1, 2022

I'm a fan of this. I experimented with similar in react-ogl, where I wanted to do away with hardcoded types (and instead iterate over the three/ogl namespace). There wasn't a way to distinguish what were DOM JSX types and three (they overlap a bit), so a separate interface (that would extend into JSX.IntrinsicElements) was useful to restrict instances to three/ogl properties.

@hmans
Copy link
Contributor

hmans commented Jul 1, 2022

As an aside, is there a specific need to have all the hardwired types in three-types.ts? Could these not be provided by a mapped type? (Outside of the scope of this PR, but it is something I had wondered about on more than one occasion.)

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Jul 11, 2022

As an aside, is there a specific need to have all the hardwired types in three-types.ts? Could these not be provided by a mapped type? (Outside of the scope of this PR, but it is something I had wondered about on more than one occasion.)

There isn't a way to automate this in TypeScript without reflection. You'd need the class reference to draw args and class type for properties & methods (e.g. Node<Element, typeof Element>.

An interesting thing about the way this is solved in this PR, is that augmenting ThreeElements in turn will also augment JSX.IntrinsicElements:

This isn't completely backward-compatible in the case of custom elements, since previous user-land usage for extend via JSX.IntrinsicElements won't be reflected in the new interface. Maybe this should be pointed to v9?

@AndrewPrifer
Copy link
Contributor Author

@CodyJasonBennett sure, extending JSX.IntrinsicElements won't also extend ThreeElements, but I'm not sure that would be necessary to qualify as backward-compatible, because previous user-land usage doesn't have the concept of ThreeElements anyway, no? They could upgrade to this version and keep extending JSX.IntrinsicElements and they wouldn't know the difference. Or am I misunderstanding your point?

@gsimone
Copy link
Member

gsimone commented Jul 11, 2022

This isn't completely backward-compatible in the case of custom elements, since previous user-land usage for extend via JSX.IntrinsicElements won't be reflected in the new interface.
I don't understand this statement, how would this be a problem? 🤔 Old extensions would still work by extending IntrisicElements, it should be fine.

A good +1 for the v9 major would be that this way we could also safely port drei & other echosystem libs with a hard dependency on v9 and change the extensions to target ThreeElements

edit: Ops, double teamed with @AndrewPrifer 😆

@CodyJasonBennett
Copy link
Member

sure, extending JSX.IntrinsicElements won't also extend ThreeElements, but I'm not sure that would be necessary to qualify as backward-compatible, because previous user-land usage doesn't have the concept of ThreeElements anyway, no? They could upgrade to this version and keep extending JSX.IntrinsicElements and they wouldn't know the difference. Or am I misunderstanding your point?

My concern is that this is not strictly backward-compatible in the case of custom elements. Although this is a minor in R3F, it needs to be communicated by consuming libraries that a code change is required for custom elements to be respected via this new interface, semver from peer deps aside. This is something we'll want to note in R3F's release notes as JSX.IntrinsicElements is effectively deprecated.

@CodyJasonBennett
Copy link
Member

I had meant to include this in 8.1 alongside #2339. Is this good to publish in 8.2?

@joshuaellis
Copy link
Member

Is this going to be a breaking change for libs that look at IntrinsicElements? Just so I know for rs

@CodyJasonBennett
Copy link
Member

No, but we'll want to make extending ThreeElements in v9 the default. We can later use this interface to bolster type-safety in extend and internal APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Typescript issues to do with TS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants