Skip to content

Commit

Permalink
Pass Systrace and Refresh as globals
Browse files Browse the repository at this point in the history
Summary:
The `Systrace` and `Refresh` dependencies are injected into the `metroRequire` implementation by assigning the values to e.g. `require.Systrace = ...`.

The issue with this approach is that some `require` implementations might not support extending the `require` object or doing so results in a degraded performance. An example where this is the case is Hermes where changing the `require` object forces Hermes to opt out of the static require optimization.

This diff extends Metro so that the `Systrace` and `Refresh` implementation can either be injected by assigning to `require.Systrace` or by exposing the implementation in the global scope. It further changes the `Systrace` and `Refresh` modules to inject the instances using the global scope instead of extending `require`.

Changelog:
[Internal][Changed] - Expose Systrace and ReactRefresh as globals instead of extending require.

Reviewed By: motiz88

Differential Revision: D25693381

fbshipit-source-id: 254d66d43e7a56d3310cf1a17d5146b8d1307562
  • Loading branch information
Micha Reiser authored and facebook-github-bot committed Jan 4, 2021
1 parent 4d9908b commit 144830b
Showing 1 changed file with 26 additions and 12 deletions.
38 changes: 26 additions & 12 deletions packages/metro-runtime/src/polyfills/require.js
Original file line number Diff line number Diff line change
Expand Up @@ -365,13 +365,9 @@ function loadModuleImplementation(
throw moduleThrewError(moduleId, module.error);
}

// `metroRequire` calls into the require polyfill itself are not analyzed and
// replaced so that they use numeric module IDs.
// The systrace module will expose itself on the metroRequire function so that
// it can be used here.
// TODO(t9759686) Scan polyfills for dependencies, too
if (__DEV__) {
var {Systrace, Refresh} = metroRequire;
var Systrace = requireSystrace();
var Refresh = requireRefresh();
}

// We must optimistically mark module as initialized before running the
Expand All @@ -385,7 +381,7 @@ function loadModuleImplementation(
}
try {
if (__DEV__) {
// $FlowFixMe: we know that __DEV__ is const and `Systrace` exists
// $FlowIgnore: we know that __DEV__ is const and `Systrace` exists
Systrace.beginEvent('JS_require_' + (module.verboseName || moduleId));
}

Expand Down Expand Up @@ -428,7 +424,7 @@ function loadModuleImplementation(
}

if (__DEV__) {
// $FlowFixMe: we know that __DEV__ is const and `Systrace` exists
// $FlowIgnore: we know that __DEV__ is const and `Systrace` exists
Systrace.endEvent();

if (Refresh != null) {
Expand Down Expand Up @@ -481,7 +477,6 @@ if (__DEV__) {
beginEvent: (): void => {},
endEvent: (): void => {},
};

metroRequire.getModules = (): ModuleList => {
return modules;
};
Expand Down Expand Up @@ -528,7 +523,7 @@ if (__DEV__) {
return;
}

const {Refresh} = metroRequire;
const Refresh = requireRefresh();
const refreshBoundaryIDs = new Set();

// In this loop, we will traverse the dependency tree upwards from the
Expand Down Expand Up @@ -823,8 +818,7 @@ if (__DEV__) {
) {
window.location.reload();
} else {
// This is attached in setUpDeveloperTools.
const {Refresh} = metroRequire;
const Refresh = requireRefresh();
if (Refresh != null) {
const sourceName = modules.source?.verboseName ?? 'unknown';
const failedName = modules.failed?.verboseName ?? 'unknown';
Expand Down Expand Up @@ -929,3 +923,23 @@ if (__DEV__) {

global.__accept = metroHotUpdateModule;
}

if (__DEV__) {
// The metro require polyfill can not have module dependencies.
// The Systrace and ReactRefresh dependencies are, therefore, made publicly
// available. Ideally, the dependency would be inversed in a way that
// Systrace / ReactRefresh could integrate into Metro rather than
// having to make them publicly available.

var requireSystrace = function requireSystrace() {
return (
global[__METRO_GLOBAL_PREFIX__ + '__SYSTRACE'] || metroRequire.Systrace
);
};

var requireRefresh = function requireRefresh() {
return (
global[__METRO_GLOBAL_PREFIX__ + '__ReactRefresh'] || metroRequire.Refresh
);
};
}

0 comments on commit 144830b

Please sign in to comment.