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

[NCL] Fix skia web support #19717

Merged
merged 1 commit into from Oct 28, 2022
Merged

[NCL] Fix skia web support #19717

merged 1 commit into from Oct 28, 2022

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Oct 27, 2022

Why

ncl doesn't support @shopify/react-native-skia on web and publishing ci job is also failed: https://github.com/expo/expo/actions/runs/3313396371/jobs/5471317891
close ENG-6839

How

  • add WithSkiaWeb wrapper for web to wait for canvaskit loading
  • add dangerouslyAddModulePathsToTranspile to transpile skia files for the nullish coalescing operators

Test Plan

  • ncl on web + skia
  • for publish ncl web ci, test yarn build:web

Checklist

@expo-bot expo-bot added the bot: passed checks ExpoBot has nothing to complain about label Oct 27, 2022
@Kudo Kudo marked this pull request as ready for review October 27, 2022 06:15
@Kudo Kudo requested review from EvanBacon and Simek October 27, 2022 06:16
@Simek
Copy link
Collaborator

Simek commented Oct 27, 2022

@EvanBacon Do you think is there a different way we can fix that, or support nullish coalescing on Web, which will work out-of-the box for other libraries? Refs: #19677.

@EvanBacon
Copy link
Contributor

@Simek transpiling node modules is something we always want to do less of. The node modules should be shipped pre-transpiled.

@Simek
Copy link
Collaborator

Simek commented Oct 27, 2022

Yeah, make sense. I will add the note about this config overwrite to our docs, since the Skia seems to recommend another approach to tackle web:

@gabrieldonadel
Copy link
Member

Yeah, make sense. I will add the note about this config overwrite to our docs

@Simek @EvanBacon Just suggesting here but I found that many of the libs not compatible with Expo web are just lacking this pre-transpilation step and having docs about that (maybe a guide?) would be super nice. I am available to help you folks if that's ok as well

@Simek
Copy link
Collaborator

Simek commented Oct 28, 2022

I think that you will need to revert 66c59ea in this changeset now @Kudo.

@gabrieldonadel It would be awesome if you can help with that, I think that we should add the note about web config in the Installation section of https://docs.expo.dev/versions/latest/sdk/skia/ page. We don't have the general web troubleshooting guide or page, but maybe this additional config overwrite can also be mentioned on https://docs.expo.dev/guides/customizing-webpack/ page.

@Kudo
Copy link
Contributor Author

Kudo commented Oct 28, 2022

after revisiting the skia issue, it does actually go through some transpile process when publishing package on npm. they use react-native-builder-blob to build typescript into esm and commonjs. ideally they could transpile the nullish coalescing operators, so user don't have to add the dangerouslyAddModulePathsToTranspile in their config. i could create an issue for them.

i will merge this pr and revert 66c59ea first for sdk release.

@Kudo
Copy link
Contributor Author

Kudo commented Oct 28, 2022

close ENG-6839

@linear
Copy link

linear bot commented Oct 28, 2022

@Kudo Kudo merged commit c18d616 into main Oct 28, 2022
@Kudo Kudo deleted the @kudo/fix-skia-web branch October 28, 2022 14:08
@Kudo
Copy link
Contributor Author

Kudo commented Oct 28, 2022

Shopify/react-native-skia#1030

Kudo added a commit that referenced this pull request Oct 31, 2022
# Why

fix Shopify/react-native-skia#1030 for sdk 47

# How

- there're no meaningful native changes from 0.1.155 to 0.1.157, so we just update the _bundledNativeModules.json_
- revert the NCL webpack workaround from #19717

# Test Plan

- tested on sdk 47 expo go + @shopify/react-native-skia@0.1.157
- NCL ci passed
- NCL - `yarn build:web`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants