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

@Shopify/react-native-skia/web #2394

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

JonnyBurger
Copy link
Contributor

@JonnyBurger JonnyBurger commented Apr 20, 2024

Built with API compatibility in mind, this should not break any existing ways to use Skia.

Idea

Have a @Shopify/react-native-skia/web import which does not import React Native or Reanimated at all 🎉

This was not possible without removing support for the React Native asset loading logic, so there is also a @Shopify/react-native-skia/react-native-web package which still allows you to import assets RN-style. You would use this if you also use React Native Web, hence this name. If you use @Shopify/react-native-skia/react-native-web, you still need the Webpack override.

This will make RN Skia more plug and play on the web, because less Webpack configuration is needed. Granted you still need an override for CanvasKit.wasm, but maybe in the future with canvaskit-js, we can support the web with no config.

Why

Currently, using it on the Web has friction:

  • Importing @Shopify/react-native-skia/src/web as a TSX file is uncommon on the web and may require configuring the bundler to transpile it.
  • In my case, it causes a type error because TypeScript wants to typecheck @Shopify/react-native-skia/src/web.ts, even though it is in node_modules and skipLibCheck is enabled. Skia source files becomes part of the user TypeScript project. Only way to get rid of the error for me was to use a .js extension.
  • Webpack override is necessary to disable React Native imports. Those shouldn't be there in the first place.
  • Webpack override is necessary to configure looking to resolve for .web.ts files. It's uncommon on the web.

How it works

Uses the Bun bundler to bundle both package/src/index.ts and package/src/web.ts into one entry point and removes all RN stuff.
I replace some modules with other ones that I defined to do this. I could have used the .web.ts convention but feared this could break existing Web workflows. This means Web users import everything from 1 entry point:

// Old way (still works):
import {LoadSkia} from '@Shopify/react-native-skia/src/web'
import {Canvas} from '@Shopify/react-native-skia'
// New way (no Webpack override necessary):
import {LoadSkia, Canvas} from '@Shopify/react-native-skia/web';

Those bundles are being placed into lib/web/pure.js and lib/web/react-native-web.js (arbitrary decision).
Two TypeScript files pure.d.ts and react-native-web.d.ts were placed mapping to the type declarations generated from source.

To make the @Shopify/react-native-skia/web import actually work, a web.js and react-native-web.js file was placed in the root of the package. The convention nowadays is to use the "exports" field, but I fear that once React Native is going to add support for it, the React Native version may break (cannot import any modules that are not explicitly defined in "exports"...)

You need bun to create the 2 bundles. It's very fast though (only takes 0.1sec).

@wcandillon wcandillon self-requested a review April 21, 2024 06:33
@feresr
Copy link

feresr commented Jun 10, 2024

This would be great!
is this likely to be submitted soon, even if flagged as 'experimental'?

@wcandillon
Copy link
Contributor

@feresr yes this is what we are inclined to do. This come with a really big caveat the import name will be different but we are still considering all of our options there.

@wcandillon
Copy link
Contributor

@JonnyBurger @feresr The only issue is the asset resolving on Web right? I am thinking of disabling it on Web completely and offer another package if people want to load assets on web? What do you think?

@JonnyBurger
Copy link
Contributor Author

@wcandillon Yes, asset resolving like in React Native is not supported on @Shopify/react-native-skia/web. Only regular images are allowed.

But there is @Shopify/react-native-skia/react-native-web – so if you want to get rid of it on the web, we can just delete this entry point

@wcandillon
Copy link
Contributor

This is very compelling. I will build a test package from this branch and play with it a bit.
As mentioned, we could ship this into the package as a test and then work our way up on having it the official bundle for web.

Below are some thoughts:

  1. I suggest the following: lib/module/index.js would be the bun generated bundle (so it would keep the name module name). This assume that React Native Web would only be supported with metro. Which I think might be fair tradeoff.
  2. Could we update the plugin to use top level async NativeSetup.ts that would make the CanvasKit loading completely transparent and would be huge improvement for web installation.
  3. is it possible to throw an error if bun is not installed on the machine that runs the script?

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.

3 participants