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

[Help] JSBI to BigInt in webpack #231

Open
celluj34 opened this issue Mar 28, 2023 · 14 comments
Open

[Help] JSBI to BigInt in webpack #231

celluj34 opened this issue Mar 28, 2023 · 14 comments

Comments

@celluj34
Copy link

Hello;

I am not using babel, but webpack, to compile my TS code into JS. My total app size is gigantic due to how I have to create the entrypoints for my app. I am curious if not having to bundle JSBI would improve my bundle size. Is there a way to include the jsbi-to-bigint package in webpack?

@12wrigja
Copy link
Contributor

You might want to take a look at https://www.npmjs.com/package/source-map-explorer which can help visualize where bytes in your chunks come from.

https://webpack.js.org/loaders/babel-loader/

Seems there is a community loader for Babel in Webpack, and you should be able to load and configure the plugin this way.

@celluj34
Copy link
Author

Ah, yes, I should have inspected the output first. Sorry about that.

It looks like out of the 293.91KB from node_modules, 144.36KB is from @js-temporal/polyfill/dist/index.esm.js and 33.2KB is from jsbi/dist/jsbi-umd.js.

So it appears that Temporal + JSBI accounts for ~52% of my dependencies. Is this expected? I don't know how complex the polyfill is, but that seems excessive. Is there any advice for reducing the package size?

@12wrigja
Copy link
Contributor

I don't know how complex the polyfill is

We do run the polyfill library source through an optimizer (Terser) before shipping it to try and reduce our bundle size, but:

  • this polyfill does aim to be faithful to the Temporal spec, which itself is large and complicated
  • even when publishing es modules, I don't believe that a bundler/optimizer is able to tree-shake away unused pieces due to the way the polyfill is structured - see because many Temporal classes can create each other. I explained that somewhat here: Split Temporal polyfill components #235 (comment).
  • One potentially large chunk of code that you aren't using is the non-ISO calendar support. The Temporal spec only mandates that there be calendar support for the ISO calendar, but we also ship implementations for various other calendars. We could potentially do better here.

If you don't need bigint compatibility, I would be interested to see what code-size numbers you get with the Babel plugin configured. That should drop code-size a fair bit both by removing JSBI from the bundle entirely but I'd expect the source attributed to Temporal to maybe drop in size a bit as well as the rewritten version of all the JSBI operations is likely smaller.

@celluj34
Copy link
Author

One potentially large chunk of code that you aren't using is the non-ISO calendar support.

Actually I am using an extremely small chunk of the library - basically just converting strings and numbers into PlainDates, doing a few comparisons, adding days, calling .toString, that's about it.

I would be interested to see what code-size numbers you get with the Babel plugin configured.

I am currently not using babel at all (only webpack + typescript with ts-loader), so I was trying to see if I could port the jsbi-babel plugin into a native ts-loader transformer, but it's looking to be way more complex than babel (plus I wasn't able to get ts-loader to actually see the files from /node_modules/). I've opened an issue at GoogleChromeLabs/babel-plugin-transform-jsbi-to-bigint#15 but that repo is really low activity so I don't think I'll be able to do that any time soon.

I was playing around with the other polyfill library (https://github.com/fullcalendar/temporal) and they don't ship with JSBI, but instead rely on native BigInt support. With IE now officially deprecated, I don't know if I personally see a reason to ship with a BigInt polyfill since it appears that most major browsers support BigInt already, but I'm certainly not going to tell you how to ship your own library.

With how little of the library I'm using, at this point I'm content to use temporal-polyfill and wait to see if I can get some assistance for the ts-loader transformer. If there's anything else I can try (that won't take up hours and hours) I'd be more than glad to give it a shot.

@12wrigja
Copy link
Contributor

I set up a barebones webpack project that can use the ts-loader for TS sources and Babel to run the JSBI plugin, and the important part seems to be adding

{
  test: /\.(js)$/,
  use: {
    loader: 'babel-loader',
    options: {
      plugins: ["transform-jsbi-to-bigint"]
    },
  }
}

to the webpack.config.js file. This configures the plugin to run over every JS file in the compilation - this could probably be refined if you wanted by adjusting the test (see https://webpack.js.org/configuration/module/#condition).

I'm not sure how ts-loader fits into this - are you also configuring the loader to look at node_modules JS files? The TS loader is designed to load TS sources and compile them with the typescript compiler, and it might be that it "can't see" the files in node_modules because those aren't TS sources anymore - they've already been converted to JS and typings. Note that we do publish our TS sources in our package but those would not be useful to pass to ts-loader as those aren't the sources you would actually be building against.

If you would rather not run Babel at all during development it should be relatively easy to configure that in the webpack config, only running this transform in production builds where the code-size and runtime benefits would be worth the build setup cost.

We are also considering ways to ship a bundle that is more modern that might help here, which would have the JSBI plugin run over the project sources at build time, potentially via a separate export path. See #155. If configuring this sort of a dual-package system is something you have expertise in, we would appreciate your help over there.

@celluj34
Copy link
Author

celluj34 commented Apr 16, 2023

Wow, awesome, thank you! I really don't mean for you to "do it for me", but I absolutely appreciate the assistance! I'll put my webpack.config.js down below.

are you also configuring the loader to look at node_modules JS files?

Not explicitly? See in the wbpack config, ts-loader is only loading .ts files, but we do tell webpack to resolve .js files too. I'm no webpack expert, but our output does build and run, so I'm not sure if that's a satisfactory answer for you.

If you would rather not run Babel at all during development...

Ideally we'd have our local and prod builds be as similar as possible, but for sure we can exclude this if we need to.

If configuring this sort of a dual-package system is something you have expertise in, we would appreciate your help over there.

Unfortunately I do not. I see there's a #167 already; something like that would be perfect for my use-case.

As for my webpack setup, here's the weird part. It doesn't matter include babel or not, the entire temporal polyfill gets included twice, but the JSBI dependency gets put into the 'polyfill' chunk... Again, not a webpack expert, but that seems very strange. Maybe we don't have it set up correctly, idk.

When I do include the babel plugin, I can see that the this library gets split into its constituent pieces, probably due to the modifications to the source files. According to the source-map-explorer, it looks like I save about 3KB in the polyfill itself, plus the ~33KB from excluding JSBI, but the size on disk is larger with JSBI removed. More strangeness, but (maybe) not related to temporal-polyfill itself.

export default async (env, options) => {
  const devMode = options.mode === "development";

  const entry = {
    polyfill: ["core-js/stable", "regenerator-runtime/runtime"],
    taskpane: "./src/taskpane/taskpane.ts",
    functions: "./src/functions/functions.ts", // put this last so that hot reloading works
  };

  const additionalEntryPoints = await getAdditionalEntryPoints();

  additionalEntryPoints.forEach(({ name, path }) => {
    entry[name] = path;
  });

  return {
    devtool: devMode ? "eval-source-map" : "source-map",
    entry: entry,
    resolve: {
      extensions: [".ts", ".html", ".js"],
    },
    module: {
      rules: [
        // {
        //   test: /\.js$/,
        //   use: {
        //     loader: "babel-loader",
        //     options: {
        //       plugins: ["transform-jsbi-to-bigint"],
        //     },
        //   },
        // },
        {
          test: /\.ts$/,
          loader: "ts-loader",
          options: {
            getCustomTransformers: () => ({ before: [tsNameof] }),
          },
        },
        {
          test: /\.html$/,
          loader: "html-loader",
          options: {
            preprocessor: (content) => (devMode ? content : transform(content)),
            sources: {
              //ignore 'src' attributes pointing to anything in the 'assets/' folder
              urlFilter: (attribute, value) => attribute !== "src" || !/^\/assets\//.test(value),
            },
          },
        },
        {
          test: /\.css$/,
          type: "asset/resource",
        },
      ],
    },
    output: {
      path: resolve(__dirname, "dist"),
      publicPath: "/",
      clean: true,
    },
    plugins: [
      new CustomFunctionsMetadataPlugin({
        input: "./src/functions/functions.ts",
        output: "functions.json",
      }),
      new TypedocWebpackPlugin({
        functionsFilename: "src/functions/functions.ts",
        tsConfigFilename: "./tsconfig.json",
        outputFilename: "assets/functions-typedoc.json",
      }),
      new HtmlWebpackPlugin({
        template: "./src/taskpane/taskpane.html",
        filename: "taskpane.html",
        chunks: ["polyfill", "taskpane", "functions"],
        minify: "auto",
        xhtml: true,
      }),
      ...additionalEntryPoints
        .filter(({ hasHtml }) => hasHtml)
        .map(
          ({ name, htmlName, htmlPath }) =>
            new HtmlWebpackPlugin({
              template: htmlPath,
              filename: htmlName,
              chunks: ["polyfill", name],
              minify: "auto",
              xhtml: true,
            })
        ),
      new CopyWebpackPlugin({
        patterns: [
          {
            from: "assets/*",
            //don't copy anything from the SVG folder
            filter: (filePath) => !filePath.includes("/svg/"),
            //don't transform when in development or if it's not in the list
            transform: (content, fullPath) => (devMode || !assetsToTransform.includes(basename(fullPath)) ? content : transform(content)),
          },
          {
            from: "manifest.xml",
            //don't transform when in development
            transform: (content) => (devMode ? content : transform(content)),
          },
          {
            from: "favicon.ico",
          },
        ],
      }),
    ],
    devServer: await getDevServerOptions(devMode),
    optimization: {
      splitChunks: {
        chunks: "all",
      },
    },
  };
};

WITHOUT BABEL PLUGIN:

image

WITH BABEL PLUGIN:

image

@12wrigja
Copy link
Contributor

but the size on disk is larger with JSBI removed.

That is very unexpected. I'll try and set some time aside this week to look at the repo I cobbled together to see if that is also true for my setup.

Thanks for posting your webpack config - that helps a lot. Is the project itself opensource? If so, I'd be happy to fork a branch with your work in progress stuff and take a more direct look when I have time, but if not no worries. I'll try and replicate something similar based on what I see here.

I didn't expect that the representation of this polyfill in the source map explorer output to change so drastically between the two configurations... it sort of seems like in the "no Babel plugin" case the sourcemaps from our dist files (that point back to the TS files) are being ignored. There also appears to be two entire copies of the polyfill in there, but the sources referenced are identical? That also seems very odd to me.

@celluj34
Copy link
Author

Is the project itself opensource?

Unfortunately not, but I can tell you it's an Excel add-in which contains a taskpane, custom functions, and ribbon commands. The details of the app itself aren't important - I can try to strip down our code and see if I can include something you can look at. Maybe by whittling down I can find somewhere where it's being duplicated.

In both cases (babel and no babel) they are identical. So I'm not sure what's causing that - I don't think it happens for any other package but every other package is also much smaller. Maybe if it were tree-shakable it would be better? I don't know.

@12wrigja
Copy link
Contributor

A minimal repro of your build setup including the tsconfig.json, webpack config files, package.json (with other libraries removed is fine), commands/scripts run, etc would be greatly helpful.

@celluj34
Copy link
Author

I've found the problem already. Our app works by defining the 'taskpane.html' and any dialog html pages. If I exclude all of the dialogs, then only one copy is produced. If I include a dialog that does not reference Temporal, only one copy is produced. When I include one or more dialog window pages, then a second copy is produced.

I can't imagine this is a js-temporal problem, but the other Temporal polyfill library (fullcalendar/temporal) does not exhibit this problem, nor any other npm package (that I can tell).

I'm not sure if that helps, but I will keep digging. I didn't have time last night, but I'll try to get a minimal repo for you soon.

@12wrigja
Copy link
Contributor

That is very interesting indeed. My guess is that the way the dialog code is written is causing bundlers to think you need both the ESM and (either CJS or UMD) versions of Temporal at the same time.

Does

"browser": "./dist/index.umd.js",
ever show up in your build somehow?

@celluj34
Copy link
Author

No. I did a search for umd and nothing came up, either in my source files or dist files. I did find @js-temporal/polyfill/dist/index.esm.js but no UMD files.

@celluj34
Copy link
Author

I can force all node_modules into a single chunk with the splitChunks.cacheGroups option, but is that really the best way?

    optimization: {
      splitChunks: {
        chunks: "all",
        //solves duplicate package problem but at what cost?
        cacheGroups: {
          vendor: {
            test: /[\\/]node_modules[\\/]/,
            name: "vendors",
            chunks: "all",
          },
        },
      },
    },

@espretto
Copy link

espretto commented May 8, 2024

it might be that it "can't see" the files in node_modules

Hi, I managed to pick and choose from the npm-packaged typescript modules by applying this change #283. By transpiling them in my stack I could also apply the JSBI babel plugin to further reduce bundle-size.

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