Skip to content

fix: snapshot handling in react-native#3605

Merged
2heal1 merged 5 commits intomodule-federation:mainfrom
jbroma:fix/strict-browser-check
Mar 25, 2025
Merged

fix: snapshot handling in react-native#3605
2heal1 merged 5 commits intomodule-federation:mainfrom
jbroma:fix/strict-browser-check

Conversation

@jbroma
Copy link
Member

@jbroma jbroma commented Mar 17, 2025

Description

#3587 introduced a change which breaks loading remotes in React Native. The root cause of this is that the isBrowserEnv check isn't strict enough, and RN is getting recognized as browser env. Stricter version is based on implementation from popular (+1M weekly downloads) npm package browser-or-node

In addition to that change, snapshot handling in RN needs a slight adjustments in runtime-core - we need to check for remoteEntry outside browser env too to ensure the remotes load properly.

Thanks @borisyankov for spotting this issue!

Related Issue

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation.

@changeset-bot
Copy link

changeset-bot bot commented Mar 17, 2025

🦋 Changeset detected

Latest commit: c529386

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

This PR includes changesets to release 30 packages
Name Type
@module-federation/runtime-core Patch
@module-federation/sdk Patch
@module-federation/runtime Patch
@module-federation/devtools Patch
@module-federation/data-prefetch Patch
@module-federation/dts-plugin Patch
@module-federation/enhanced Patch
@module-federation/esbuild Patch
@module-federation/managers Patch
@module-federation/manifest Patch
@module-federation/modern-js Patch
@module-federation/nextjs-mf Patch
@module-federation/node Patch
@module-federation/retry-plugin Patch
@module-federation/rsbuild-plugin Patch
@module-federation/rspack Patch
@module-federation/storybook-addon Patch
@module-federation/utilities Patch
@module-federation/webpack-bundler-runtime Patch
@module-federation/bridge-react-webpack-plugin Patch
@module-federation/bridge-react Patch
@module-federation/bridge-vue3 Patch
@module-federation/runtime-tools Patch
@module-federation/modernjsapp Patch
@module-federation/inject-external-runtime-core-plugin Patch
@module-federation/third-party-dts-extractor Patch
@module-federation/bridge-shared Patch
@module-federation/error-codes Patch
create-module-federation Patch
website-new 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

@netlify
Copy link

netlify bot commented Mar 17, 2025

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit c529386
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/67de85df762369000834d37d
😎 Deploy Preview https://deploy-preview-3605--module-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ScriptedAlchemy
Copy link
Member

Is there not a way to detect the env better? is it still a browser? or is there some global that we can use to say detect isReactNative?

@borisyankov
Copy link

const isReactNative = navigator.product === "ReactNative";
image

@jbroma
Copy link
Member Author

jbroma commented Mar 18, 2025

const isReactNative = navigator.product === "ReactNative";

yes, exactly like Boris mention we could go with explicit check for React-Native like it was done in #2812

function isReactNativeEnv(): boolean {
  return (
    typeof navigator !== 'undefined' && navigator?.product === 'ReactNative'
  );
}

@jbroma
Copy link
Member Author

jbroma commented Mar 18, 2025

@ScriptedAlchemy I've updated the PR with explicit RN env check 👍

@zmzlois
Copy link

zmzlois commented Mar 18, 2025

LGTM
image

@ScriptedAlchemy
Copy link
Member

perfect, ill run it past team - but looks good to me.

Do you need a canary release in the meantime?

@ScriptedAlchemy ScriptedAlchemy changed the base branch from main to is-react-native March 18, 2025 20:06
@ScriptedAlchemy ScriptedAlchemy requested a review from 2heal1 March 18, 2025 20:12
@jbroma
Copy link
Member Author

jbroma commented Mar 18, 2025

perfect, ill run it past team - but looks good to me.

Do you need a canary release in the meantime?

Thanks!

No need for the canary, I've tested the changes locally and it's all good 👍

@ScriptedAlchemy ScriptedAlchemy changed the base branch from is-react-native to main March 18, 2025 21:28
@jbroma jbroma mentioned this pull request Mar 19, 2025
9 tasks
@2heal1 2heal1 merged commit 047857b into module-federation:main Mar 25, 2025
4 checks passed
@2heal1 2heal1 mentioned this pull request Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants