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

fix empty export on web #6171

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

EvanBacon
Copy link
Contributor

Summary

React Native Babel preset uses a loose plugin to convert imports to commonjs, we're investigating switching to a more standard approach in Expo CLI here expo/expo#30005 which aligns with how Metro is used at Meta.

When running on the kitchen sink, I found that there was an invalid expression in reanimated where it exports null and then destructures null. This error didn't occur previously because the commonjs plugin deferred access to the null object until later.

Before

var _Math = _$$_REQUIRE(_dependencyMap[0]);

// Later ...

_Math.add

This coincidentally didn't throw because the deferred code is never accessed.

After

var add = _$$_REQUIRE(_dependencyMap[0]).add;

This throws because the null is accessed when the module is loaded.

Test plan

Bundle a Metro web project with experimentalImportSupport enabled:

config.transformer.getTransformOptions = async () => ({
  transform: {
    experimentalImportSupport: true,
    inlineRequires: false,
  },
});

Then run npx serve dist -> Error was there but not anymore.

@piaskowyk
Copy link
Member

Thank you for the fix and detailed description!

@piaskowyk piaskowyk enabled auto-merge June 28, 2024 07:34
@piaskowyk piaskowyk added this pull request to the merge queue Jun 28, 2024
Merged via the queue into software-mansion:main with commit b7eb791 Jun 28, 2024
5 of 6 checks passed
@tomekzaw tomekzaw mentioned this pull request Jun 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jun 28, 2024
## Summary

This PR fixes the following linting error that happened on main after
#6171 was merged.

<img width="436" alt="Screenshot 2024-06-28 at 10 08 41"
src="https://github.com/software-mansion/react-native-reanimated/assets/20516055/74985d60-f774-46ea-bad1-2d11845fe9d5">

## Test plan
@EvanBacon EvanBacon deleted the @evanbacon/fix-web-export branch July 2, 2024 02:01
@codeion
Copy link

codeion commented Jul 5, 2024

Hi,
It's breaking build with vite bundler, maybe to consider this patch...
react-native-reanimated+3.13.0.patch

@djaffer
Copy link

djaffer commented Jul 9, 2024

WARNING in ./node_modules/react-native-reanimated/lib/module/createAnimatedComponent/createAnimatedComponent.js:265
export 'RNRenderer' (imported as 'RNRenderer') was not found in '../platform-specific/RNRenderer' (module has no exports)
263 | viewConfig = null;
264 | } else {

265 | var hostInstance = RNRenderer.findHostInstance_DEPRECATED(component);
266 | if (!hostInstance) {
267 | throw new Error('[Reanimated] Cannot find host instance for this component. Maybe it renders nothing?');
268 | }

EvanBacon added a commit to expo/expo that referenced this pull request Jul 23, 2024
# Why

> Most of the original PR has been landed in
#30417

- Metro bundler does not have any form of graph-wide tree shaking
built-in. This can lead to bundle sizes that are larger than they need
to be, especially in the case of icon libraries.
- This PR (which is partially an RFC) aims to add a system for removing
unused imports and exports from a production bundle.

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Leverage the new "optimize graph" mode to perform holistic
optimizations in the serializer. #30417
- Tree shaking can be enabled with the new env var
`EXPO_UNSTABLE_TREE_SHAKING=1`.
- This PR also adds a "barrel expansion" system which attempts to
replace `export * from '...'` statements with `export { View, Image, etc
} from '...'` statements to enable more graph reduction.
- Empty modules (containing only comments and directives) are removed
from the bundle.
- Side-effecty imports (imports with zero specifiers, `import "foo"` and
`import {} from "foo"`) are preserved unless marked otherwise by a
`package.json`'s `sideEffects` field.
- Custom imports such as `require.context` and `require.resolveWeak` are
left in place.
- Removed exports with source will trigger a recrawl to ensure any new
unused imports/exports are recursively removed.
- The feature is artificially forced to only run when
`EXPO_UNSTABLE_TREE_SHAKING=1` is set, and when bundling for production.
Tree shaking is disabled for SSR bundles. I may expand this to be
web-only too just due to a lack of continuous testing for native
runtimes.
- Added some external fixes
software-mansion/react-native-reanimated#6171
#30010
#29980
#29964
#29963
software-mansion/react-native-gesture-handler#2972

# Further work

Some ideas that I've had but aren't implemented in this PR:

- Use the worker farm to parallelize transformation in the serializer.
This could be tricky because we'd need to convert the AST to a serial
format then re-parse it back.
- ~~Fork collectDependencies to fully track the import/exports so we can
reuse the same import crawling code and references. This would also
speed up the bundling as we could parallelize and cache the pass.~~
#30140
- Use a totally custom syntax for matching import/exports, e.g.
`_$$_IMPORT_DEFAULT, _$$_IMPORT_ALL` (and some new export equivalents)
to reduce the number of transforms we run in the serializer. All
import/export's and the iife wrap could be run at transform-time and
we'd simply be mutating a valid JS bundle in the serializer. This would
be very challenging though and make it harder for users to contribute to
the implementation.
- ~~Upstream the hack on transformSyncRequire to support require.context
with one parameter. facebook/metro#1129
- Detect if a module is CJS according to Node.js heuristic, e.g. `.cjs`,
package exports, etc. then opt it out of tree shaking.
- [ ] update source maps

# Test Plan

- Added an e2e tree shaking test for web. 
- Added mini-metro tests to account for the known and expected tree
shaking behaviors.
- Added mini-metro tests for side-effect handling (need more here).
- Ensured all apps in expo/apps can be transformed and run with tree
shaking enabled.

---------

Co-authored-by: Expo Bot <[email protected]>
Co-authored-by: Aman Mittal <[email protected]>
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