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

inject option should only "inject once" for each entrypoint #1557

Open
osddeitf opened this issue Aug 27, 2021 · 5 comments
Open

inject option should only "inject once" for each entrypoint #1557

osddeitf opened this issue Aug 27, 2021 · 5 comments

Comments

@osddeitf
Copy link

osddeitf commented Aug 27, 2021

Hello, and thanks for this awesome project.

I'm trying to create my own build tool for React.
I want the compile time as small as possible, so i use React CDN in my index.html, with my entrypoint index.tsx compiled to index.js.

When i use jsx with the option minifyIdentifiers: true, React.createElement is leave as is.
That's correct since "react" package is excluded from the bundle by external: ["react"], and no React identifiers are imported.

Bundle size matters, so that's why i need to use inject option with the following shim.ts, with jsxFactory option changed to _jsx, and the minifyIdentifiers option works as intended:

const _jsx = React.createElement
export { _jsx }

But when I tried to turn off minifyIdentifiers to inspect what's generated to figure it out, it looked like this:

// src/shim.ts
var _jsx;
var init_shim = __esm({
  "src/shim.ts"() {
    _jsx = React.createElement;
  }
});

// template/index.tsx
init_shim();

// template/app.tsx
init_shim();

// dist/navigation/Navigation1.js
init_shim();

I ran with only one entrypoint, but there're multiple init_shim().
Now that I actually understand how inject option works.
But isn't it a huge performance penalty if my shim.ts do some little heavy computation, and I need to compile a lot of files into a single entrypoint?
And what if there's some side effect inside shim.ts (bad practice anyway)?
That's the problem.

Proposal

We should only need to have one injection like init_shim() above for each entrypoint.
That'll makes sense, because the purpose of inject is to "inject global variables" only, and it'll be called first in any entrypoint.
It'll only be a breaking change for those who are relying on side effects of each "injection".

Environment & configuration

  • esbuild: v0.12.23
  • Configurations:
{
  bundle: true,
  external: ['react'],
  entryPoints: [
    'template/index.tsx'
  ],
  inject: ["src/shim.ts"],
  outdir: "template/dist/",
  minifySyntax: false,
  minifyIdentifiers: false,
  minifyWhitespace: false,
  tsconfig: "template/tsconfig.json",

  format: 'iife',
  platform: 'browser',
  target: ["es6"],
  jsxFactory: "_jsx",
}
@osddeitf
Copy link
Author

Actually, my codes have a pollyfill for import { useState } from 'react'.

tsconfig.json

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "react": ["react-pollyfill.ts"]
    }
  }
}

react-pollyfill.ts

declare var React: any
module.exports = React

When i tried to modify react-pollyfill.ts like this:

declare var React: {
  useState: (x: any) => any,
  useEffect: (x: any) => any,
}

export var useState = React.useState
export var useEffect = React.useEffect

The result is different:

(function() {
  // src/shim.production.ts
  var _jsx = React.createElement;

  // template/react-pollyfill.ts
  var useState = React.useState, useEffect = React.useEffect;

not having a lot of codes:

(function() {
  var __create = Object.create;
  var __defProp = Object.defineProperty;
  var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
  var __getOwnPropNames = Object.getOwnPropertyNames;
  var __getProtoOf = Object.getPrototypeOf, __hasOwnProp = Object.prototype.hasOwnProperty;
  var __markAsModule = function(target) {
    return __defProp(target, "__esModule", { value: !0 });
  };
  var __esm = function(fn, res) {
    return function() {
      return fn && (res = (0, fn[Object.keys(fn)[0]])(fn = 0)), res;
    };
  };
  var __commonJS = function(cb, mod) {
    return function() {
      return mod || (0, cb[Object.keys(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports;
    };
  };
  var __reExport = function(target, module, desc) {
    if (module && typeof module == "object" || typeof module == "function")
      for (var keys = __getOwnPropNames(module), i = 0, n = keys.length, key; i < n; i++)
        key = keys[i], !__hasOwnProp.call(target, key) && key !== "default" && __defProp(target, key, { get: function(k) {
          return module[k];
        }.bind(null, key), enumerable: !(desc = __getOwnPropDesc(module, key)) || desc.enumerable });
    return target;
  }, __toModule = function(module) {
    return __reExport(__markAsModule(__defProp(module != null ? __create(__getProtoOf(module)) : {}, "default", module && module.__esModule && "default" in module ? { get: function() {
      return module.default;
    }, enumerable: !0 } : { value: module, enumerable: !0 })), module);
  };

  // src/shim.production.ts
  var _jsx, init_shim_production = __esm({
    "src/shim.production.ts": function() {
      _jsx = React.createElement;
    }
  });

  // template/react-pollyfill.ts
  var require_react_pollyfill = __commonJS({
    "template/react-pollyfill.ts": function(exports, module) {
      init_shim_production();
      module.exports = React;
    }
  });

  // template/index.tsx
  init_shim_production();

  // template/app.tsx
  init_shim_production();

  // dist/navigation/Navigation1.js
  init_shim_production();
...

How's that happened? is it because i mixing between esm and commonjs modules?

@hyrious
Copy link

hyrious commented Aug 28, 2021

is it because i mixing between esm and commonjs modules?

Yes. A simple rule: all commonjs codes have side effects so they should be executed everytime when they are imported.

As in your problem, you don't have to use inject as it is used to polyfill global names instead of import "react". The tsconfig/path is enough. Just make sure all your code is esm.

// tsconfig.json
{ "compilerOptions": { "paths": { "react": ["./react-shim.js"] } } }

// react-shim.js
export default React
export var useState = React.useState

// a.js
import React, { useState } from "react"
import "./b"
console.log(React, useState('a'))

// b.js
import { useState } from "react"
console.log(useState('b'))

// esbuild a.js --bundle
(() => {
  // react-shim.js
  var react_shim_default = React;
  var useState = React.useState;

  // b.js
  console.log(useState("b"));

  // a.js
  console.log(react_shim_default, useState("a"));
})();

@osddeitf
Copy link
Author

so, if multiple injection is commonjs expected behavior, i'll close this issue then

@osddeitf
Copy link
Author

but, as i know, when multiple files require() the same module, the side effect only runs once. So that's still a problem.

@evanw
Copy link
Owner

evanw commented Sep 9, 2021

This is similar to an existing issue: #475. The linker would ideally merge duplicate imports. However, this gets complicated with top-level await so doing this is not as straightforward as it would initially seem.

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

No branches or pull requests

3 participants