Improve bundle size of library

[rudnitskih]

The most simple configuration for the react loader is 36KB (not gziped version):

Suggestions:

• Understand why tree-shaking is not working
• Eliminate lodash from the bundle, just couple usage of this

The file which I analyzed:

import {ModuleRegistry, ReactLazyComponent} from 'react-module-container';
import {externalNameServersWidgetLoaderName, externalNameServersWidgetName} from '../../constants';
import PropTypes from 'prop-types';
import {NameServersBlockProps} from '../pages/Tutorials/Generic/Dns/CopyNameservers/NameServersBlock';

export interface NameServersWidgetLoaderProps extends NameServersBlockProps {
  debug: boolean;
  staticsBaseUrl: string;
  domainName: string;
  locale?: string;
  translations?: any;
}

export class NameServersWidgetLoader extends ReactLazyComponent {
  constructor(props: NameServersWidgetLoaderProps) {
    const {staticsBaseUrl, debug} = props;
    const jsPath = staticsBaseUrl + (debug ? 'name-servers-widget.bundle.js' : 'name-servers-widget.bundle.min.js');
    const cssPath = staticsBaseUrl + (debug ? 'name-servers-widget.css' : 'name-servers-widget.min.css');
    super(props, {
      files: [jsPath, cssPath],
      component: externalNameServersWidgetName,
      unloadStylesOnDestroy: true,
      resolve: async (): Promise<NameServersBlockProps> => {
        return {};
      },
    });
  }
}

(NameServersWidgetLoader as any).propTypes = {
  staticsBaseUrl: PropTypes.string.isRequired,
  domainName: PropTypes.string.isRequired,
  currentNameServers: PropTypes.any,
  translations: PropTypes.any,
  locale: PropTypes.any,
  nameServers: PropTypes.any,
  debug: PropTypes.bool,
  onCopySuccess: PropTypes.func,
  onCopyError: PropTypes.func,
};

ModuleRegistry.registerComponent(externalNameServersWidgetLoaderName, () => NameServersWidgetLoader);

[rudnitskih]

@shahata @shlomitc What do you think about these suggestions?

[shlomitc]

Hey @rudnitskih ! I'm not the owner of the library, but both of your suggestions are relevant and important and PR will be welcome. I think @ronenst is the relevant person.

To pinpoint the problem, let's understand how the library is consumed. From looking in some project, I see consumers import the ModuleRegistry like this:

import { ModuleRegistry } from 'react-module-container';

But in other places I see it is externalized:

<script src="https://static.parastorage.com/unpkg/react-module-container@1.0.1570/dist/statics/react-module-container.bundle.min.js"></script>

Possible action items

1. If the module is always externalized in a project - then tree shaking is not relevant (because you always need all of the code).
2. If the module is always bundled, then some work can be done to make it tree-shakable.
3. lodash bundling can improve

Making Tree Shaking work:

Tree Shaking does not work in this case because the project is using an old version of yoshi (1.2.0) that doesn't export requirable es-modules. Upgrading to yoshi@4 and adding another module entry to the package.json will do the trick https://wix.github.io/yoshi/docs/guides/export-es-modules

Optimizing lodash dependencies

Regarding lodash - I see that the library is already cherry-picking only the relevant dependencies, for example import uniqueId from 'lodash/uniqueId'; So this means we bundle only what's needed. Regarding your suggestion to remove it, I think it's ok to remove functions that are already basic in the language forEach, but I wouldn't put much time on useful functions that will need reimplementation.

[rudnitskih]

@shlomitc Thank you for the quick response!

Tree-shaking issue

Our project is not using an external script to load this library. I checked the Account Manager and Bussiness Manager apps and also didn't find react-module-container in their HTML templates. I personally don't like external scripts because it complicates setup (describing additional stuff in package.json and index.ejs) and maintenance (syncing version in package.json and index.ejs). I would like to use this way only for heavy libraries (react, mobx, etc).

But it is up to the owner of this library and should be described in docs.

Lodash issue

I understand that eliminating lodash increases the amount of code that owners should support. My point is for such scripts as loaders are so important to be small as much they can. And we can save 15KB just replacing all places with vanillajs. _.set and _.unset have over complicated implementation inside for such simple case which solved inside react-module-container. BTW, uniqueId just 3 lines of code: https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L16170

Also, we need to define browsers which this library supports to decide what to do with this task.

[ronenst]

react-module-container is meant to be provided globally once by the hosting platform and externalized by consumers. 10KB gzipped seems a reasonable size for a core lib.

Account Manager and Business Manager provide this library externally, but admittedly it's not easy to find: https://github.com/wix-private/business-manager/blob/master/business-manager-web/src/client/workarounds/initModulesDependencies.ts#L8 https://github.com/wix-private/account-manager/blob/master/account-manager-client/src/services/loadModules.ts#L24

But this is documented and is also supposed to exist in the test-app/generators. https://github.com/wix-private/business-manager/blob/master/docs/integration-step-by-step.md#setup-external-dependencies https://github.com/wix-private/account-manager/blob/master/docs/globals.md

If you have an AM or BM project and is not set to use it as an external then please do.

[rudnitskih]

Thanks, @ronenst for the comments. Unfortunately, our project (Domains app) is integrated as an iframe inside AM and BM. But I will take into consideration this when we will do real integration.

But remember this is an open-source library and maybe it can be important for other projects. This issue is not so important for our project, I am ok to close it. Just shared my investigation.

[ronenst]

~I feel I should also clarify that all lodash methods in this project are loaded via manual tree shaking, this lib does not import the entire lodash library but only the code that is needed, so for example uniqueid is imported as: import uniqueId from 'lodash/uniqueId';~

~Which only loads these three lines: https://cdn.jsdelivr.net/npm/lodash@4.17.15/uniqueId.js~

~So the cost in size should be not much more than any proper custom implementation.~

nvm, I see that was already mentioned.