Fix type conflict with @types/react #233

hasparus · 2019-10-25T16:24:19Z

Connected issues: #172, #11, #10

Problem

Described in #172.

react-three-fiber types conflict with @types/react what forces users leveraging TypeScript type system to skip typechecking libraries.

Main change

I have removed audio and line from IntrinsicElements interface.
@types/react "was winning" and shadowing three-fiber audio and line definitions, so they weren't usable for users with types for React.
This is a small, but breaking change for people using react-three-fiber and react without @types/react, what is possible. I'm not sure how much of JavaScript projects would be affected, because VSCode automatic type acquisition can get @types/react for JavaScript users.

I've copycatted the lie from React Native and added components export with values typed as React.FC<...>, which are strings at runtime.

Using this makes react-three-fiber independent of global JSX.IntrinsicElements
Allows seamlessly adding actual components with render props etc.

import { components as t } from 'react-three-fiber'

const _ = (
  <t.Mesh
    onUpdate={mesh => console.log(mesh.geometry)}
    onClick={() => console.log('click')}
    onPointerOver={() => console.log('hover')}
    onPointerOut={() => console.log('unhover')}>
    <t.BoxBufferGeometry attach="geometry" args={[1, 1, 1]} />
    <t.MeshNormalMaterial attach="material" />
  </t.Mesh>
)

It would be pretty cool to allow to

import { Mesh } from 'react-three-fiber/components'`

but I didn't want to complicate the PR.

More changes

Added jest to unit test components a bit.
Removed @types/three, because it's deprecated. THREE ships its own types.
- Dev dependency on three will provide types during development.
- Peer dependency will give them to the library user.
- If a the library user uses THREE in version without types, they can install proper version of @types/three themselves.
Disabled '@typescript-eslint/no-empty-interface'. This rule doesn't know if you extend a mapped type which is the case for ThreeFiberComponents.
yarn typecheck didn't work for me 😔
- I added --emitDeclarationOnly false to make noEmit work.

Notes

I think components.tsx is a little bit more complicated than it has to be.
We could map over THREE exports without losing type info (no Object.entries) and instead of type asserting entire dict as any as ThreeFiberComponents, assert string value to a function component with proper props.
I'm not sure if I'm capable enough to simplify it soon :c

Deployed demo with my changes:

package.json

drcmda · 2019-10-30T23:46:42Z

thanks for this! it's lots to take in, so, the change here is that there's an optional way to get components and it isn't that much weight? native elements are still working?

i would also prefer react-three-fiber/components btw i think that would make imports cleaner

hasparus · 2019-10-31T09:58:52Z

i would also prefer react-three-fiber/components btw i think that would make imports cleaner

That would also contribute to autoimport, so when I write the text editor can propose me to import Mesh from three or a Mesh from react-three-fiber/components.

native elements are still working

Yup, I didn't touch the reconciler.

I removed audio and line from types. I've noticed that the editor support I get on them prefers their declarations from @types/react. They still work at runtime though.
This change can fail CI build for somebody who's using react-three-fiber without @types/react.
It's kind of "breaking" I'd say, so it could probably be a part of next major release.

it isn't that much weight

Ouch. It's good you've mentioned it.

The components object is just a string for every three export.

I imported three (which is huge) and ran Object.entries on it to generate the components export.
I didn't check yet, but this should break treeshaking on three.

I'm not sure if this is a problem, though. Doesn't react-three-fiber imports entire three already?
https://github.com/react-spring/react-three-fiber/blob/master/src/reconciler.tsx#L186

Possible solutions:

This is a long shot, because I'm not sure if it's a problem, but I'll run through possible solutions.

I'll try to measure bundle size in a demo app to make sure.

Using a proxy to do the mapping from Key to key.
Cons:
- Higher runtime cost than reading from object?
- This would make import { Mesh } from 'react-three-fiber/components not plausible.

Generating this mapping at build-time.

// components.tsx
export const Mesh = 'mesh' as any as ThreeFiberComponents['Mesh'];
export const BoxBufferGeometry = 'boxBufferGeometry' as any as ThreeFiberComponents['BoxBufferGeometry'];
export const MeshNormalMaterial = 'meshNormalMaterial' as any as ThreeFiberComponents['MeshNormalMaterial'];

Pros:

Makes named imports of components trivial.

Cons:

Contributes to bundle size and adds another build step.
Needs to be regenerated after changes of three public API?

drcmda · 2019-10-31T15:24:33Z

if it's under /components i'm ok with it, it wouldn't matter if it pulls the entirety of three because it doesn't treeshake anyway. the only way i am aware of is using aliases, this is what i do to compress my projects and that would work with these wrapper components as well.

the only sore spot is

Needs to be regenerated after changes of three public API?

that one is something i wanted to avoid. tying the lib to a particular three version, or being forced to track their release cycle is not ideal.

one more question i have is about the extend function. how would that go?

currently:

import { extend } from 'react-three-fiber'
import { SpecialThing } from 'xyz'
extend({ SpecialThing })

<specialThing />

i know it would work like always, but wouldn't it then be confusing that you still have to rely on native elements?

i've been thinking about another special object, like this:

import { SpecialThing } from 'xyz'

<new object={SpecialThing} args={[...]} {...props} />

primitive = channels pre-existing objects into the graph
construct = constructs class prototypes into new objects

hasparus · 2019-10-31T21:21:15Z

the only sore spot is
Needs to be regenerated after changes of three public API?

Yeah, this is a big disadvantage. Especially because three is at version 0.x since forever and this page can be kinda scary when you have a large project.

that one is something i wanted to avoid. tying the lib to a particular three version, or being forced to track their release cycle is not ideal.

I totally agree with it. This is a selling point of react-three-fiber.

I think I can do something that enables named imports from react-three-fiber/components and doesn't introduce a hard dependency on particular three version.

one more question i have is about the extend function. how would that go?

This could work too

import { extend } from 'react-three-fiber'
import { SpecialThing } from 'xyz'

/* extend could return "specialThing" with a fake wrapper component type */
const { SpecialThing: Special } = extend({ SpecialThing })

<Special />

hasparus · 2019-10-31T21:34:25Z

i've been thinking about another special object, like this:
import { SpecialThing } from 'xyz'

<new object={SpecialThing} args={[...]} {...props} />

I think you can't get intrinsic elements to be generic, so this wouldn't autocomplete/typecheck.

import { New } from 'react-three-fiber'
import { SpecialThing } from 'xyz'

<New object={SpecialThing} args={[...]} {...props} />

Using a component for that (regardless if the logic is actually inside of it or in the reconciler) would be easier IMHO.

I've made a quick example in TypeScript playground https://links.typescript.social/5J6n

hasparus · 2019-10-31T21:43:01Z

I'm gonna try to get named imports from react-three-fiber/components working tomorrow morning.

@codynova You might be interested in this PR.

hasparus · 2019-11-01T10:18:11Z

I'm gonna try to get named imports from react-three-fiber/components working tomorrow morning.

I can't use module.exports nor export = , because they can't be compiled to ESNext.

So if I'm not wrong, the only way to get named imports of object entries, would be to transform this object into a module at build time.
(Object.entries(components).map(([k, v]) => `export const ${k} = ${v}`).join('\n')).

I see two problems with it.

a small one — I really like this import syntax more. Especially, since it allows to import both entire namespace and particular imports at the same time (like import React, { useState }), but adding a build-time script for it is probably vanity.
a big one — ties react-three-fiber to one three version.

drcmda · 2019-11-01T12:47:25Z

and proxies? i mean, this would basically be an add-on anyway so people know what they're getting into. this wouldn't tie it too close to the main lib and we're not dependent to a threejs version. not sure about types, but you're the pros, it can probably be done in some way or another.

hasparus · 2019-11-03T20:53:34Z

and proxies?

Well, since we can't export object as a module it makes no difference if it's computed at the startup of the app (like I do with components ATM) or on the go with a proxy. I'd guess that plain object would be better performance-wise.

Shipping without ES6 modules would be a regression and not worth the gain IMO.

but you're the pros

By no means :D

drcmda · 2019-11-03T23:15:58Z

so, if we have an optional react-three-fiber/components esm export, not tied to the main export. and it's primed to three@latest and we find someone that likes to update it regularly or leave it to the community to update it via pr's, would it then be good to go?

hasparus · 2019-11-04T09:33:58Z

Yeah, I can add a script to package.json to dump what is right now components object into a components.tsx module so a PR to update it would be pretty easy to do.

Classes exported by three don't change that often so a github action / a cron job would probably be a overkill.

drcmda · 2019-11-04T09:59:08Z

yes, please, no cron jobs :-D it could just use the threefiber main three devdependency to generate this file yarn build.

hasparus · 2019-11-04T19:45:13Z

I couldn't just print all components object entries as a module, because there are deprecated three exports (SplineCurve3, CubeGeometry) and things like CanvasRenderer with which I don't really know what to do. I feel that filtering them out in value world would be harder than excluding them in types, so I dumped the type instead.

I have also added Primitive fake component with a type that probably matches applyProps behavior.

drcmda · 2019-11-08T13:45:25Z

what's your take on this? should we merge? is it safe?

hasparus · 2019-11-08T22:25:47Z

I've used it a little bit already, though I can't share the project. I'm gonna do the pool game tutorial using this and deploy the results this week. Maybe I'll find some problems along the way.

This is technically a breaking change for people using audio and line TypeScript without @types/react (I'm not sure how many of them exist, but I'd assume that at least 1?), so maybe it should go on 4.x/next branch?

drcmda · 2019-11-08T22:38:17Z

seriously, who's using our types without @types/react 🙂but i would be ok with it. any other ideas that could go into a major?

hasparus · 2019-11-19T03:26:35Z

Well, I didn't finish my pool game, but I've fixed merge conflicts and adapted /components to the changes in package build.

package.json

drcmda · 2019-11-19T08:05:41Z

im gonna add a v4 branch today, is it possible to target this one in this pr instead of master?

I've noticed you're publishing just `dist` right now. That's super nice. I'm changing types outdir, so `components.d.ts` stands right beside `components.js` what enables typed _subdirectory-style_ import. --- Additional changes: - Add "pack-dist" script to package.json I've been using it a lot, and I suppose it would be cool to download packed package with a GitHub Action or sth like that in Pull Requests.

hasparus · 2019-12-04T07:41:57Z

I've fixed the conflict and rebased to v4. I didn't have time to get deep into v4 changes, but I love what I've seen already. Especially the Dom component, because this is such a common use case.

drcmda · 2020-01-07T14:03:22Z

@hasparus i am considering releasing v4, it's been in testing for a longer time now and the features are pretty good. should we take a longer time still for types or do you think it's all ready to go?

hasparus · 2020-01-07T22:48:35Z

It should be okay, though I’d probably test it again after the merge with v4 just in case.

hasparus commented Oct 26, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

drcmda mentioned this pull request Nov 9, 2019

🔮 v4 collecting ideas (suggestions welcome) #240

Closed

6 tasks

hasparus commented Nov 19, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

hasparus changed the base branch from master to v4 November 19, 2019 10:09

hasparus added 8 commits December 4, 2019 08:12

Add three-types.test.tsx to check if three.types.ts works

7896a1e

Comment out audio and line from typings

fc1c81f

Map over THREE exports to build a type for fake components

1d0300f

Map over three export to create dictionary with element names

47394f7

Test components object

2500cb6

Named export fake components

e1e2582

Add fake component for New

f79e7a1

Fix components export

b5c61b7

Merge branch 'v4' into react-types-conflict

16627dd

drcmda merged commit cfa4470 into pmndrs:v4 Jan 8, 2020

hasparus deleted the react-types-conflict branch January 8, 2020 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type conflict with @types/react #233

Fix type conflict with @types/react #233

hasparus commented Oct 25, 2019 •

edited

Loading

drcmda commented Oct 30, 2019 •

edited

Loading

hasparus commented Oct 31, 2019 •

edited

Loading

drcmda commented Oct 31, 2019 •

edited

Loading

hasparus commented Oct 31, 2019

hasparus commented Oct 31, 2019

hasparus commented Oct 31, 2019

hasparus commented Nov 1, 2019

drcmda commented Nov 1, 2019

hasparus commented Nov 3, 2019

drcmda commented Nov 3, 2019

hasparus commented Nov 4, 2019

drcmda commented Nov 4, 2019

hasparus commented Nov 4, 2019

drcmda commented Nov 8, 2019

hasparus commented Nov 8, 2019

drcmda commented Nov 8, 2019

hasparus commented Nov 19, 2019

drcmda commented Nov 19, 2019

hasparus commented Dec 4, 2019

drcmda commented Jan 7, 2020

hasparus commented Jan 7, 2020

Fix type conflict with @types/react #233

Fix type conflict with @types/react #233

Conversation

hasparus commented Oct 25, 2019 • edited Loading

Problem

Main change

More changes

Notes

drcmda commented Oct 30, 2019 • edited Loading

hasparus commented Oct 31, 2019 • edited Loading

Possible solutions:

drcmda commented Oct 31, 2019 • edited Loading

hasparus commented Oct 31, 2019

hasparus commented Oct 31, 2019

hasparus commented Oct 31, 2019

hasparus commented Nov 1, 2019

drcmda commented Nov 1, 2019

hasparus commented Nov 3, 2019

drcmda commented Nov 3, 2019

hasparus commented Nov 4, 2019

drcmda commented Nov 4, 2019

hasparus commented Nov 4, 2019

drcmda commented Nov 8, 2019

hasparus commented Nov 8, 2019

drcmda commented Nov 8, 2019

hasparus commented Nov 19, 2019

drcmda commented Nov 19, 2019

hasparus commented Dec 4, 2019

drcmda commented Jan 7, 2020

hasparus commented Jan 7, 2020

hasparus commented Oct 25, 2019 •

edited

Loading

drcmda commented Oct 30, 2019 •

edited

Loading

hasparus commented Oct 31, 2019 •

edited

Loading

drcmda commented Oct 31, 2019 •

edited

Loading