V5 linting #610
V5 linting #610
Conversation
| export function useCannon({ ...props }, fn) { | ||
| const ref = useRef() | ||
| // Get cannon world object | ||
| const world = useContext(context) | ||
|
|
||
| // Instanciate a physics body | ||
| const [body] = useState(() => new CANNON.Body(props)) | ||
| const [body] = useState(new CANNON.Body(props)) | ||
| const [instanced, setInstanced] = useState(false) | ||
| useEffect(() => { | ||
| // Call function so the user can add shapes | ||
| fn(body) | ||
| if (!instanced) { | ||
| fn(body) | ||
| setInstanced(true) | ||
| } | ||
| // Add body to world on mount | ||
| world.addBody(body) | ||
| // Remove body on unmount | ||
| return () => world.removeBody(body) | ||
| }, deps) | ||
| }, [instanced, fn, world, body]) | ||
|
|
There was a problem hiding this comment.
The way we we injecting deps fails linting. We actually aren't using that option anyway here, so I removed it to simplify this example. In order to prevent fn(body) from being called more than needed, I had to introduce the instanced behavior seen here. If you load up the example in the browser, it still works as intended with these changes.
There was a problem hiding this comment.
i think the demo could even be replaced by one of the use-cannon demos. but that's for later.
| @@ -1,13 +1,13 @@ | |||
| import React from 'react' | |||
| import { Canvas, useThree, useFrame } from 'react-three-fiber' | |||
| import { render } from '../../../src/targets/css2d' | |||
There was a problem hiding this comment.
This was actually pointing to the wrong path.
| @@ -105,8 +106,12 @@ export default () => ( | |||
| onCreated={({ gl }) => { | |||
| gl.gammaInput = true | |||
| gl.gammaOutput = true | |||
| gl.toneMapping = THREE.Uncharted2ToneMapping | |||
There was a problem hiding this comment.
This toneMapping was removed recently in Three.js
mrdoob/three.js#19606
I am following the replacement that was offered in this PR
| @@ -1,8 +1,9 @@ | |||
| import React, { Suspense, createRef, useEffect, useRef } from 'react' | |||
| import { Canvas, Dom, useFrame } from 'react-three-fiber' | |||
| import { Html } from 'drei' | |||
There was a problem hiding this comment.
Updated since Dom is no longer a thing in react-three-fiber and was delegated to drei
| @@ -147,7 +147,7 @@ export { Clock } from 'three/src/core/Clock.js' | |||
| // export { CubicInterpolant } from 'three/src/math/interpolants/CubicInterpolant.js'; | |||
| export { Interpolant } from 'three/src/math/Interpolant.js' | |||
| // export { Triangle } from 'three/src/math/Triangle.js'; | |||
| export { _Math as Math } from 'three/src/math/Math.js' | |||
| // export { _Math as Math } from 'three/src/math/Math.js' | |||
There was a problem hiding this comment.
Math.js no longer exists in this form from three.js
| "test": "jest", | ||
| "typecheck": "tsc --noEmit --emitDeclarationOnly false --strict --jsx react", | ||
| "typegen": "tsc && mv dist/src/* dist && rm -rf dist/src || true", | ||
| "typegen:components": "npx ts-node src/components/generateExports.ts", | ||
| "copy": "copyfiles package.json readme.md LICENSE dist && json -I -f dist/package.json -e \"this.private=false; this.devDependencies=undefined; this.optionalDependencies=undefined; this.scripts=undefined; this.husky=undefined; this.prettier=undefined; this.jest=undefined; this['lint-staged']=undefined;\"", | ||
| "pack-dist": "cd dist && yarn pack && mv *.tgz .." | ||
| }, | ||
| "husky": { | ||
| "hooks": { | ||
| "pre-commit": "pretty-quick --staged --pattern \"**/*.*(js|jsx|ts|tsx)\"" |
There was a problem hiding this comment.
pretty-quick is great for getting started, but it doesn't actually run eslint. It only does formatting. I set up lint-staged to correctly lint + format any staged changes.
| @@ -104,17 +99,21 @@ | |||
| "eslint": "^7.5.0", | |||
| "eslint-config-prettier": "^6.11.0", | |||
| "eslint-plugin-import": "^2.22.0", | |||
| "eslint-import-resolver-alias": "^1.1.2", | |||
| "eslint-plugin-prettier": "^3.1.4", | |||
There was a problem hiding this comment.
This dependency was missing which was one of the causes for prettier not formatting correctly on save in vscode. It also was preventing lint errors from surfacing via yellow squiggles. There were errors in the terminal output from Eslint which was halting it.
| state.current.pointer.emit('pointerCancel', event) | ||
| if (prepare) prepareRay(event) | ||
| // commenting this out as I believe it is unneccessary and causes a recursive dependency | ||
| // if (!hits) hits = handleIntersects(event, () => null) |
There was a problem hiding this comment.
I'm going to look further into this before merging, but I don't believe this invocation is actually necessary.
| callback(ref.current) | ||
| invalidate() | ||
| } | ||
| }, dependents) | ||
| }, [callback, dependents, invalidate, ref]) | ||
| return ref | ||
| } |
There was a problem hiding this comment.
One way to still use dependencies that are passed in is to follow the usePrevious model. Something to note is that hook dependencies use shallow comparison checks which doesn't often play nicely with arrays since mutating an array doesn't cause a new object ref.
|
I'm going to go ahead and merge this chunk of fixes first and tackle the |
Our eslint + prettier configuration needed a few adjustments as it was not surfacing hints or working with formatOnSave in vscode.
There were quite a few linting errors that were artifacts collected over time and I tried to address these.
Everything seems to still work fine so far, but I have to fix the hook dependencies in
canvas.tsx. The way that we are declaring these dependencies doesn't align with thereact-hooks/exhaustive-depsrule and will require some refactoring. Attempting the suggested autofix is a breaking change with the existing behavior.I'm going to continue working on
canvas.tsx, but I wanted to get eyes on this earlier since it has a wide breadth of changes.