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

feat(typescript): Basic TypeScript support #59

Merged
merged 15 commits into from
Apr 26, 2019
Merged

Conversation

setsun
Copy link
Sponsor Contributor

@setsun setsun commented Apr 5, 2019

What

Adds TypeScript support to react-three-fiber by adding a .tsconfig.json, and changing the file extensions to .ts / .tsx. This gives us some decent inferred type-checking out of the box with tsc.

Also add a new npm script called typecheck, which calls tsc --noEmit --jsx react src/*, to perform static type-checking on the fly, which may be useful if something like CircleCI is setup for continuous integration.

This is also potentially useful because Babel will compile the files, regardless of if it passes TypeScript type checks or not. 🤷‍♂️

Example output for yarn run typecheck is as follows:

src/canvas.tsx:31:19 - error TS2554: Expected 5 arguments, but got 3.

31       if (camera) applyProps(cam, camera, {})

@drcmda would love to get your thoughts on the approach, and if you have some ideas on a public types/ interface you would want end-users to use.

To-do

  • Fix existing TypeScript errors from inferred types.
  • Create types/ folder, providing public types for react-three-fiber to be used by end-users.

@drcmda
Copy link
Member

drcmda commented Apr 5, 2019

To me it looks good. Would it be hard to fully type it out? Starting with canvas props and hooks?

I wonder if it would be possible at all to also incorporate threejs typings.

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 5, 2019

Sounds good to me, I'm in the process of fixing the types in canvas.tsx. I think it would be possible to start with Canvas hooks and props and put those types in the types/ folder similar to react-spring.

I believe three typings should be possible as well, given that @types/three is moved from devDependencies to dependencies, so end-users will have those types available to them on installation.

src/canvas.tsx Outdated
@@ -58,7 +58,7 @@ export const Canvas = React.memo(
const [bind, size] = useMeasure()
const [intersects, setIntersects] = useState([])
const [raycaster] = useState(() => new THREE.Raycaster())
const [mouse] = useState(() => new THREE.Vector2())
const [mouse] = useState(() => new THREE.Vector3())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my, what did i do there ... good catch! 😅

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The promise of TypeScript is real!

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 12, 2019

Just an update, currently on vacation now, but I plan on re-visiting this afterwards!

@drcmda
Copy link
Member

drcmda commented Apr 12, 2019

@setsun awesome! enjoy your time! :)

@drcmda
Copy link
Member

drcmda commented Apr 23, 2019

@setsun looks really good! do you need help with something? or is this ready to go, other than the small lockfile conflicts?

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 23, 2019

@drcmda this is mostly good to go for a good first start. I'll add some documentation on the new npm scripts.

I'll update this tonight, and I'll let you know when it's good for review!

Copy link
Sponsor Contributor Author

@setsun setsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left some comments on some of the important changes. I think in a follow-up PR I would love to refine some of the foundational work here.

There are still a few type errors, but babel will still compile the code all the same. This seems fine to me at the moment, since these are type errors that have existed in the past, and we can incrementally fix these as we go forward. Most of these are three.js errors that I'm a little unfamiliar with.

Let me know if you have any questions! I believe after review, this should be possible to merge in.

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -206,6 +269,7 @@ export const Canvas = React.memo(

for (let hit of hits) {
let stopped = { current: false }

fn({
Copy link
Sponsor Contributor Author

@setsun setsun Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given my current knowledge of three.js it was hard to type the Object that is passed back in the handleIntersects callback fn. I've left this alone for now, but would love to revisit this.

@@ -0,0 +1,100 @@
import * as THREE from 'three'
Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These *.d.ts files are generated by the npm run typegen script, and should be available for consumption by end-users. TypeScript is also pretty smart, and will generate inferred types, without any explicit types in the source code.

container.__interaction.push(instance)
}

instance.__handlers = handlers.reduce(
Copy link
Sponsor Contributor Author

@setsun setsun Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private methods like __handlers these are a little hard to type, since in the official three.js types, these aren't included in the type definitions intentionally.

I think this is to discourage usage of these properties, but we could extend these types in react-three-fiber. Still need to find out how best to do that.

@drcmda
Copy link
Member

drcmda commented Apr 25, 2019

im ok with all of them, could you make a last rebase/update to latest changes? then we can merge 🎉

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 25, 2019

@drcmda just fixed! I tried running the example from the examples/ folder, but looks like there is a runtime error on both master and this branch.

I just ran the compiled output when yarn link-ed in a personal project and it has the same functionality and works the same way.

Let me know if you have any issues, but looks good to me on my end 🎉

@drcmda drcmda mentioned this pull request Apr 26, 2019
@drcmda drcmda merged commit 58dabd9 into pmndrs:master Apr 26, 2019
@drcmda
Copy link
Member

drcmda commented Apr 26, 2019

@setsun worked for me (examples), so i merged, thank you so much!

Just the last building step crashes out (type gen)

created dist/index.cjs.js in 720ms
npm WARN lifecycle The node binary used for scripts is /var/folders/n6/2ys_sbln60d7hlwngzqzs_p80000gn/T/yarn--1556291529368-0.9420328491320966/node but npm is using /usr/local/Cellar/node/11.1.0/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.

> react-three-fiber@2.0.9 typegen /Users/drcmda/dev/react-three-fiber
> tsc  --jsx react --emitDeclarationOnly -d --declarationDir types src/*

src/canvas.tsx:261:22 - error TS2339: Property '__handlers' does not exist on type 'Object3D'.

261           if (object.__handlers) hits.push({ ...intersect, object })
                         ~~~~~~~~~~

src/canvas.tsx:334:24 - error TS2339: Property 'object' does not exist on type '{}'.

334               if (data.object.uuid !== object.uuid) {
                           ~~~~~~

src/canvas.tsx:335:26 - error TS2339: Property 'object' does not exist on type '{}'.

335                 if (data.object.__handlers.pointerOut)
                             ~~~~~~

src/canvas.tsx:336:24 - error TS2339: Property 'object' does not exist on type '{}'.

336                   data.object.__handlers.pointerOut({ ...data, type: 'pointerout' })
                           ~~~~~~

src/canvas.tsx:337:45 - error TS2339: Property 'object' does not exist on type '{}'.

337                 delete hovered.current[data.object.uuid]
                                                ~~~~~~

src/canvas.tsx:351:47 - error TS2339: Property 'object' does not exist on type 'never'.

351         if (!hits.length || !hits.find(i => i.object === data.object)) {
                                                  ~~~~~~

src/canvas.tsx:351:63 - error TS2339: Property 'object' does not exist on type '{}'.

351         if (!hits.length || !hits.find(i => i.object === data.object)) {
                                                                  ~~~~~~

src/canvas.tsx:352:20 - error TS2339: Property 'object' does not exist on type '{}'.

352           if (data.object.__handlers.pointerOut) data.object.__handlers.pointerOut({ ...data, type: 'pointerout' })
                       ~~~~~~

src/canvas.tsx:352:55 - error TS2339: Property 'object' does not exist on type '{}'.

352           if (data.object.__handlers.pointerOut) data.object.__handlers.pointerOut({ ...data, type: 'pointerout' })
                                                          ~~~~~~

src/canvas.tsx:353:39 - error TS2339: Property 'object' does not exist on type '{}'.

353           delete hovered.current[data.object.uuid]
                                          ~~~~~~

src/reconciler.tsx:152:42 - error TS2472: Spread operator in 'new' expressions is only available when targeting ECMAScript 5 and higher.

152     instance = is.arr(args) ? new target(...args) : new target(args)
                                             ~~~~~~~


Found 11 errors.

npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! react-three-fiber@2.0.9 typegen: `tsc  --jsx react --emitDeclarationOnly -d --declarationDir types src/*`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the react-three-fiber@2.0.9 typegen script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/drcmda/.npm/_logs/2019-04-26T15_12_21_099Z-debug.log
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 26, 2019

@drcmda awesome! Even though the yarn typegen has an exit error, it still generates the *.d.ts files the best it can. I'll see if I can find time to fix the remaining errors.

@setsun setsun deleted the typescript branch April 26, 2019 15:31
@drcmda
Copy link
Member

drcmda commented Apr 26, 2019

after this, will there be any chance to incorporate threejs types into the native element attribute types? or is this impossible to do?

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 26, 2019

By that do you mean what @AnteGulin was suggesting here?: #11 (comment)

I think it's something we can include in the types/ folder if that solves the issue. @AnteGulin would you be down in opening a PR?

@drcmda
Copy link
Member

drcmda commented Apr 26, 2019

I'm not a TS user, but i guess if i open <mesh position={ TS would tell me i can dump a Vector3 or an array in here? That would be amazing.

@paulmelnikow
Copy link
Contributor

Yea, you can actually specify the types of all the attributes 😁

I've started doing this, though it's only the partial subset that I'm using. I have this in a file called global.d.ts.

namespace JSX {
  import { LegacyRef } from 'react'
  import THREE from 'three'
  import { MouseEventHandler } from 'react-three-fiber'

  interface IntrinsicElements {
    group: {
      ref?: LegacyRef<THREE.Group>
      children?: Element[] | Element
      position?: THREE.Vector3
    }
    geometry: {
      name?: string
      ref?: LegacyRef<THREE.Geometry>
      vertices?: THREE.Vector3[]
      onUpdate?: (self: THREE.Geometry) => boolean
    }
    lineBasicMaterial: {
      name?: string
      color?: string
    }
    mesh: {
      name?: string
      ref?: LegacyRef<THREE.Mesh>
      onUpdate?: (self: THREE.Mesh) => boolean
      onClick?: MouseEventHandler
      onHover?: MouseEventHandler
      onUnhover?: MouseEventHandler
      children?: Element[] | Element
      geometry?: THREE.Geometry
      material?: THREE.Material
      scale?: THREE.Vector3
      position?: THREE.Vector3
    }
    sphereGeometry: {
      name: string
      args?: [number?, number?, number?, number?, number?, number?, number?]
    }
    octahedronGeometry: {
      name: string
    }
    meshBasicMaterial: {
      name: string
      color?: string | number
      opacity?: number
      transparent?: boolean
      side?: THREE.Side
      depthTest?: boolean
      depthWrite?: boolean
      args?: any
    }
    meshStandardMaterial: {
      name: string
      color?: string | number
      opacity?: number
      transparent?: boolean
      side?: THREE.Side
      depthTest?: boolean
      depthWrite?: boolean
      args?: any
    }
    meshLambertMaterial: {
      name: string
      color?: string | number
      opacity?: number
      transparent?: boolean
      side?: THREE.Side
      depthTest?: boolean
      depthWrite?: boolean
      args?: any
    }
    ambientLight: {
      name?: string
      args?: [number?, number?]
      visible?: boolean
    }
    directionalLight: {
      name?: string
      args?: [string | number | undefined, number?]
      position?: [number, number, number] | THREE.Vector3
      visible?: boolean
    }
    primitive: {
      object: THREE.Object3D
    }
  }
}

A couple questions:

  1. Would this file belong in types/ or somewhere else?
  2. What's the best place to reference the accepted parameters? I was working from the Three.js docs though maybe there is a better place.

@drcmda
Copy link
Member

drcmda commented Apr 27, 2019

But can this be automated? B/c otherwise we have to ship a HUUUGGGEEE catalogue and maintain it, too. Not to mention that technically three-fiber isn't tied to a specific three version, it can be used with any published version.

@paulmelnikow
Copy link
Contributor

It might be possible to automate it, though first I think I need to find out where the information is. 😀

For example, how can I get a full list of the supported element tags?

Is primitive the only one that is not mapped directly to THREE.Something?

@drcmda
Copy link
Member

drcmda commented Apr 27, 2019

primitive can be anything, not related to three even. you could dump regular objects into the scene graph with it. that one could be impossible to type, but it's an escape hatch.

@paulmelnikow
Copy link
Contributor

@drcmda Which elements are supported?

@aleclarson
Copy link
Contributor

aleclarson commented Apr 29, 2019

@paulmelnikow I think all elements are supported. Right @drcmda?

This gets you the available tag names:

import * as THREE from 'three'

type ThreeExports = typeof THREE

type Newable<T extends any[] = any[], U = any> = new (...args: T) => U

// The available tag names
type ThreeElements = {
  [P in keyof ThreeExports]: ThreeExports[P] extends Newable ? P : never
}[keyof ThreeExports]

As for the prop types, you might be able to use a mapped type on each class and filter out unsupported elements/props (if any) like this:

type UnsupportedElements = never

// Note: All properties are assumed to be supported (but are they?)
type SupportedProps<
  T extends Newable,
  K extends keyof ThreeExports
> = K extends any ? keyof T : never // Note: The `extends any` part is a placeholder for `extends 'a'`
                                    // (so we can filter by component name if necessary)

// The props of each supported component
type ThreeComponents = {
  [K in keyof ThreeExports]: ThreeExports[K] extends infer T
    ? T extends Newable
      ? { [P in SupportedProps<T, K>]: T[P] }
      : never
    : never
}

LMK how that works out

@aleclarson
Copy link
Contributor

I'm not seeing the types in the node_modules/react-three-fiber directory. 😢

Is there a missing build step?

@aleclarson
Copy link
Contributor

Hmm, after cloning and running yarn, I have found some TypeScript errors in src/canvas.tsx

src/canvas.tsx:261:22 - error TS2339: Property '__handlers' does not exist on type 'Object3D'.

261           if (object.__handlers) hits.push({ ...intersect, object })
                         ~~~~~~~~~~

src/canvas.tsx:334:24 - error TS2339: Property 'object' does not exist on type '{}'.

334               if (data.object.uuid !== object.uuid) {
                           ~~~~~~

src/canvas.tsx:335:26 - error TS2339: Property 'object' does not exist on type '{}'.

335                 if (data.object.__handlers.pointerOut)
                             ~~~~~~

src/canvas.tsx:336:24 - error TS2339: Property 'object' does not exist on type '{}'.

336                   data.object.__handlers.pointerOut({ ...data, type: 'pointerout' })
                           ~~~~~~

src/canvas.tsx:337:45 - error TS2339: Property 'object' does not exist on type '{}'.

337                 delete hovered.current[data.object.uuid]
                                                ~~~~~~

src/canvas.tsx:351:47 - error TS2339: Property 'object' does not exist on type 'never'.

351         if (!hits.length || !hits.find(i => i.object === data.object)) {
                                                  ~~~~~~

src/canvas.tsx:351:63 - error TS2339: Property 'object' does not exist on type '{}'.

351         if (!hits.length || !hits.find(i => i.object === data.object)) {
                                                                  ~~~~~~

src/canvas.tsx:352:20 - error TS2339: Property 'object' does not exist on type '{}'.

352           if (data.object.__handlers.pointerOut) data.object.__handlers.pointerOut({ ...data, type: 'pointerout' })
                       ~~~~~~

src/canvas.tsx:352:55 - error TS2339: Property 'object' does not exist on type '{}'.

352           if (data.object.__handlers.pointerOut) data.object.__handlers.pointerOut({ ...data, type: 'pointerout' })
                                                          ~~~~~~

src/canvas.tsx:353:39 - error TS2339: Property 'object' does not exist on type '{}'.

353           delete hovered.current[data.object.uuid]
                                          ~~~~~~

src/reconciler.tsx:152:42 - error TS2472: Spread operator in 'new' expressions is only available when targeting ECMAScript 5 and higher.

152     instance = is.arr(args) ? new target(...args) : new target(args)
                                             ~~~~~~~


Found 11 errors.

@aleclarson
Copy link
Contributor

aleclarson commented Apr 29, 2019

What is "skipLibCheck": true for in .tsconfig.json?

@setsun
Copy link
Sponsor Contributor Author

setsun commented Apr 29, 2019

@aleclarson it looks like the library needs to be npm publish-ed with the TypeScript changes before these are reflected when you install the library.

I've only had a lil time to address the remaining type issues (which don't currently block compilation). Would welcome PRs and to collab if y'all have time on the remaining issues!

"jsx": "react",
"pretty": true,
"skipLibCheck": true,
"allowJs": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe enable strict?
Will result in more errors initially, but worth for the long run.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ksjogo just fixed in #86!

export type CanvasContext = {
canvas?: React.MutableRefObject<any>
subscribers: Array<Function>
frames: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these two be numbers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subscribers is an array of callbacks. frames is an int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant to comment on the line below, as the two 0s will be Numeric Literal Types and thus only allow actual 0 as value.

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.

None yet

5 participants