Skip to content

Add ChunksToHermesBytecodePlugin#378

Merged
jbroma merged 22 commits into
callstack:mainfrom
mikeduminy:add-hermes-plugin
Jun 23, 2023
Merged

Add ChunksToHermesBytecodePlugin#378
jbroma merged 22 commits into
callstack:mainfrom
mikeduminy:add-hermes-plugin

Conversation

@mikeduminy
Copy link
Copy Markdown
Contributor

@mikeduminy mikeduminy commented Jun 16, 2023

Summary

At Klarna we've been using Re.pack for a while now, and wish to contribute back our implementation of a ChunksToHermesBytecodePlugin. This PR is based on an in-house plugin developed by @oblador.

The plugin itself re-implements tranformations found in each platform (which assume a single main chunk):

For iOS the logic is copied from react-native-xcode.sh
For Android the logic is copied from BundleHermesCTask.kt (with defaults in ReactExtension.kt)

Side note: maybe these should be extracted into the react-native CLI, or scripts, instead of having separate implementations?

Test plan

  • Add to TesterApp
  • Verify iOS (bundle, bundle and run in Release, start and run in Dev)
  • Verify Android (bundle, bundle and run in Release, start and run in Dev)
  • Verify it works as expected for the simple case

Open Resolved* questions

  1. [Resolved] Should this transformation happen inside the OutputPlugin (or AssetProcessors)?

    • There's a lot of reusable logic in those and it feels like a more natural home.

    Resolution: No, the OutputPlugin is already doing too much.

  2. [Resolved] Should we transform all chunks in this plugin, or ignore the main chunk (letting react-native transform that using hermes) and transform the rest?

    • PRO: Letting react-native transform the main chunk means we don't have to recommend that people opt-out of this (less work, more maintainable)
    • CON: It means special handling within Re.pack to ignore the main chunk and risks the transformation algorithm diverging over time (and composition of source maps). This might be okay since the main chunk is usually the most important to have transformed, so if our implementation gets out of sync with RN it might not be too bad.
    • There is no super clean way of doing this on Android. See comment in code.

    Resolution: By default we let RN transform the main chunk but the API of the plugin should allow users to also transform the entry chunk if they want.

  3. [Resolved] Alternative to (2): What if the ChunksToHermesBytecodePlugin accepted a transformMainChunk: boolean property?

    • This would allow the most flexibility

    Resolution: See (2)

Side note: maybe we should add more fine-grained control to the configuration react-native where you can opt out of the automatic hermes transformation of the bundle, but still use Hermes as the runtime.

TODO:

  • Resolve open questions
  • Figure out how to make it work with remote chunks
  • Add sensible defaults and messaging for misuse of the plugin
  • Add tests for the plugin
  • Update API docs
  • Update usage docs

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 16, 2023

🦋 Changeset detected

Latest commit: b1b7c42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@callstack/repack Minor
testerapp Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mikeduminy
Copy link
Copy Markdown
Contributor Author

I wonder if this should be added to the output plugin as an optional step. It needs to use a lot of the same logic to determine local vs remote chunks 🤔

Comment thread packages/repack/src/webpack/plugins/OutputPlugin.ts
Comment thread packages/TesterApp/ios/TesterApp.xcodeproj/project.pbxproj Outdated
Comment thread packages/TesterApp/android/gradle.properties Outdated
@mikeduminy mikeduminy marked this pull request as ready for review June 18, 2023 19:24
@mikeduminy mikeduminy changed the title Add ChunksToHermesBytecodePlugin [WIP] Add ChunksToHermesBytecodePlugin Jun 18, 2023
@jbroma
Copy link
Copy Markdown
Contributor

jbroma commented Jun 19, 2023

Hi @mikeduminy

First of all, thank you so much creating PR with this! We are very excited and are looking forward to getting this to the finish line ASAP 🚀

Regarding the open questions:

  1. Let's keep the transformation in the separate plugin as it is because:
  • OutputPlugin is something we noticed adds some convenience for the user, but introduces a lot of complexity when releasing new plugins, in the future we are probably going to deprecate it and remove it from the RePack plugin.
  • AssetsProcessor is something that deals with assets other than chunks so it's not the right place to do this transformation
  1. I think we want to leave the responsibility of transforming the main chunk to the react-native itself. It's hard to only disable the transformation without disabling the hermes engine and it might be quite confusing for the end user.

  2. Good idea, only suggestion I might have is to name it 'entryChunk' instead of 'mainChunk', please tell me what you think

What I suggest we should do starting with this plugin (and apply the same pattern for the rest of the plugins in the future):

  1. Add inputPath option to tell the plugin where to look for input
  2. Add outputPath option to tell the plugin where to place the transformed chunks. If the path is the same as inputPath it will overwrite the existing bundles with the new transformed ones.
  3. Add transformEntryChunk optional option (by default set to false) or in most cases we are going to be only omitting index.bundle (the name can be obtained through webpack configuration object available inside the plugin)
  4. Add tranformIgnore optional option to provide a list of additional ignored paths

Going forward I think this will be easier to maintain and will allow users more granular control over what gets transformed and not.

Comment thread packages/TesterApp/android/app/build.gradle Outdated
Comment thread packages/TesterApp/ios/TesterApp.xcodeproj/project.pbxproj Outdated
Comment thread packages/TesterApp/android/gradle.properties Outdated
Comment thread packages/TesterApp/webpack.config.mjs Outdated
Comment thread packages/repack/src/webpack/plugins/OutputPlugin.ts
@jbroma
Copy link
Copy Markdown
Contributor

jbroma commented Jun 19, 2023

@mikeduminy

take a look here and give this a shot, I had something like this in mind: fd06076

compare branches here

@jbroma
Copy link
Copy Markdown
Contributor

jbroma commented Jun 21, 2023

The plugin now uses assetEmitted hook instead of afterEmit so it runs before OutputPlugin which simplifies the logic of the plugin greatly.

All the transformations are now done inside build/generated directory which is the source from which OutputPlugin copies the bundles to their target directories.

Comment thread README.md
@jbroma
Copy link
Copy Markdown
Contributor

jbroma commented Jun 22, 2023

Preview of the docs available here: https://6494804b67314a519583e33b--re-pack.netlify.app/docs/configuration/plugins/chunks-to-hermes

Comment thread website/docs/configuration/plugins/chunks-to-hermes.mdx Outdated
Comment thread website/docs/configuration/plugins/chunks-to-hermes.mdx Outdated
@jbroma jbroma force-pushed the add-hermes-plugin branch from 1eb6a70 to 30ca5b3 Compare June 22, 2023 22:51
@jbroma jbroma force-pushed the add-hermes-plugin branch from 0a223ec to b1b7c42 Compare June 23, 2023 09:27
@jbroma jbroma merged commit 14afc61 into callstack:main Jun 23, 2023
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.

4 participants