Add Web Support (Take 2) #228
Conversation
|
Note, I added a guard for zero height or width on the underlying canvas since it causes obscure crashes on iOS. One thing I've thought about adding is "hiding" the web only changes a bit better that were added to |
|
is this "all" it takes to get the Web version working? :) it looks small enough :) |
|
I did go through and compartmentalize the web specific changes! This should make it a bit easier to grok the metro config updates that are required for web compatibility - it's surprisingly little, I thought it would be far more involved. I think this way should be best since the native and web example apps will be able to share example functionality. I also added an augementation of types to the global namespace so that consuming libraries shouldn't have to import Also, I did bump React Three Fiber and add a tweak to I feel bad about breaking MNIST Inference though. My implementation looks like it should be correct to me. Can you eyeball what is maybe going wrong? |
|
Thank you for doing this. The update on the MNIST example, I wouldn't worry too much about it. It makes sense that because of CanvasKit something weird might be going on here and I can look into it. When reviewing this PR some of the changes are very sensible but some changes feel unrelated, could that be? |
| useLayoutEffect, | ||
| useCallback, | ||
| } from "react"; | ||
| import type { RefObject } from "react"; |
There was a problem hiding this comment.
shouldn't this PR only add src/Canvas.web.tsx, and src/main.web.ts and that's it sorry if I am missing something.
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
this change is not related right?
| @@ -0,0 +1,34 @@ | |||
| const path = require("path"); | |||
| const root = path.resolve(__dirname, "../.."); | |||
| const r3fPath = path.resolve(root, "node_modules/@react-three/fiber"); | |||
There was a problem hiding this comment.
this import is not used here right?
|
Hey @scottmas, nice work on this! Just wondering if you’re still planning to keep going with the web support, would love to see it land 🙌 |
| import _ from "lodash"; | ||
| import { useEffect, useRef } from "react"; | ||
| import { StyleSheet } from "react-native"; | ||
| // @ts-expect-error - rn web uses @types/react-native and doesn't have types for web only exports |
There was a problem hiding this comment.
Try installing @types/react-native-web as a dev dependency, should be there 😄
| if (!navigator) { | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-expect-error | ||
| navigator = {}; | ||
| } | ||
|
|
||
| navigator.gpu = RNWebGPU.gpu; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| navigator.userAgent = "react-native"; |
There was a problem hiding this comment.
To get rid of eslint-disable, I think you should be able to use eslint-error with some comments inline
| if (!navigator) { | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |
| // @ts-expect-error | |
| navigator = {}; | |
| } | |
| navigator.gpu = RNWebGPU.gpu; | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |
| // @ts-ignore | |
| navigator.userAgent = "react-native"; | |
| if (!navigator) { | |
| // @ts-expect-error: navigator is not defined in React Native, we create a stub for WebGPU compatibility | |
| navigator = {}; | |
| } | |
| navigator.gpu = RNWebGPU.gpu; | |
| // @ts-expect-error: set a fake userAgent so code expecting it doesn’t break | |
| navigator.userAgent = "react-native"; |
| import { createContext, useContext, useEffect, useRef, useState } from "react"; | ||
|
|
||
| import type { RNCanvasContext, CanvasRef, NativeCanvas } from "./Canvas"; | ||
| import type {} from "./Canvas"; |
There was a problem hiding this comment.
Was this a mistake?
| import type {} from "./Canvas"; |
|
|
||
| export { WebGPUModule } from "./NativeWebGPUModuleWrapper"; | ||
|
|
||
| declare global { |
There was a problem hiding this comment.
Usually it’s better to put global type augmentation in a separate .d.ts file (like global.d.ts) and import it from the entry point. Keeps the runtime code cleaner and type-only stuff in one place.
3 participants
Finally got around to adding vanilla web support without a companion Expo project. Just need some tweaks to the metro.config.js to make it support web as well.
To test, go to the example app and do
yarn web, then open http://localhost:8081I was UNABLE to get MNISTInference example working fully - Nothing gets drawn to the Skia canvas, and the number 2 is always predicted. I'm honestly quite baffled on the lack of visual feedback, since
global.CanvasKitis present and no errors are generated. And having 2 always be predicted maybe is an artifact of changing how the font is loaded? Sounds like something maybe the creator of react-native-skia should have a look at...Let me know if there's any changes you'd like me to make!