Support Rollup's preserveModules

[slikts]

Current Behavior

Currently a single bundle is output by rollup and module structure is not preserved.

Desired Behavior

There should be a way to do deep imports from the packaged library like so: import FooModule from "mylib/FooModule".

Suggested Solution

Rollup has a preserveModules option that includes the module structure in the built files.

A caveat is that the library should be published from the build directory to avoid having to include the build directory in the import path (like import FooModule from "mylib/dist/FooModule").

Who does this impact? Who is this for?

Here's a simple benchmark comparing deep vs bare imports from a large library: https://github.com/slikts/deep-vs-bare-import-bench

The performance impact of having to parse unused code is non-negligible for larger libraries, and it's compounding. Tree shaking only mitigates this for the final consumer but not while developing. Larger component libraries like MUI specifically support deep imports to avoid this problem.

[swyxio]

i like this.

[TrySound]

Don't forget that material-ui has package.json with module field in each directory. preserveModules is not enough for deep imports.

[slikts]

@TrySound I mentioned this caveat about needing to publish from the build directory. This is what MUI does; it copies package.json to the build directory.

[schmod]

Even if you're not directly using deep imports, preserving the original module's file structure can help bundlers to shake out entire files when they're unused (see webpack/webpack#9337 for a full discussion).

The tl;dr; seems to be that, when we concatenate a library into a single file, Webpack (and others) are often unable to determine when it's safe to shake an external library out of the concatenated bundle.

In this scenario, it's also worth noting that you don't need to publish from the build directory. Webpack seems like it's "smart enough" to support tree-shaking from libraries similar to MUI when using shallow imports.

I'm wondering if concatenation even makes sense as a default for library authors today. In a world where most folks are using NPM and a bundler, it seems like single-file bundles should be the exception rather than the rule.

[slikts]

@schmod Thanks for the informative link; supporting tree shaking better is an additional reason to avoid bundling the library. Publishing from the build directory would still be desirable, though, because it improves the aesthetics of doing direct deep imports instead of importing re-exported modules, and deep imports help speed up the compilation and parsing when developing, so it's not just about tree shaking for the end users.

This issue is blocking me from using tsdx.

[zhaoyao91]

@slikts so what's your solution or workaround at this time? We faced similar problem :(

[markerikson]

Based on a Twitter thread from @mweststrate today, it sounds as if this option may be key to getting proper tree-shaking working:

https://twitter.com/mweststrate/status/1228056670447730688

[beeequeue]

Would just like to add that this is what is blocking us from using tsdx at work, proper tree-shaking is a very critical feature libraries should make use of! Hope it will be ready soon. 😄

[agilgur5]

@beeequeue you can already configure this in tsdx.config.js, similar to how immer used to do:

module.exports = {
  rollup(config, opts) {
    if (opts.format === 'esm') {
        config = { ...config, preserveModules: true }
        config.output = { ...config.output, dir: 'dist/', entryFileNames: '[name].esm.js' }
        delete config.output.file
    }
    return config
}

You'll have to change your package.json module to index.esm.js for this (unavoidable with output.dir) and there might still be some issues with how types are output. Might not be, but I'm not sure, #535 is still WIP after all and that's an issue I'm looking to resolve in a few places. EDIT: #691 should resolve any type output issues.
EDIT: updated per below comments to fix some bugs in the config that I forgot about.

---

Also I'm not sure that the "proper tree-shaking" part is accurate, there's a response to that tweet from Evan You (Vue's creator and lead):

From my experience it does work, albeit requires the minifier to do the actual code elimination

[beeequeue]

I have compared the result bundles of our applications (webpack+terser) with our current configurations (sideEffects: false, preserveModules: true) and the WIP tsdx migration versions, and the tsdx packages are not able to be tree-shaked (tree-shook?).

This currently results in 19kb more after gzipping, which would be a lot more if we kept migrating more components - probably eventually reaching 1MB more

I'll try that solution though, hopefully we can start using tsdx over what we have now, it's a real mess. 😄

[agilgur5]

I also don't think the use-case of "deep imports" will completely make sense for this feature, as, per another reply in that thread from Rollup's current maintainer (which itself is a reply to my own reply there), for CJS, --preserveModules results in a performance hit:

Downsides: Loading and parsing of your "bundle" by secondary bundlers will be noticeably slower. And Consumers of CJS output will likely end up having a negative performance hit as you cannot easily scope-hoist modules in CJS.

The multi-entry feature that I have pending support for in #367 is more aligned for the deep imports use-case and recommended later in that thread too. (and the remaining question in that PR is out-of-the-box support for something like "publishing from the build directory". the proposal in #437 is an alternative solution for that as well)

Also, per my response in #382, --preserveModules for CJS isn't enough for "deep imports" as it misses the CJS entry file TSDX creates for dev/prod splits. #367 appropriately creates these for each entry, so please look there for multi-entry or "deep imports" support.

[beeequeue]

We don't need this for "deep imports" - it's useful exclusively for the better tree-shaking it provides out of the box.

If we do something like import {A, B} from "pkg" and the package was built with preserveModules it will be able to tree-shake the rest of the code in the package, while if everything is bundled in one file webpack seems to be unable from our tests.

[beeequeue]

So I've experimented using Immer's config:

Immer Config

module.exports = {
  rollup: (config, options) =>
    options.format !== "esm"
      ? config
      : {
          ...config,
          // this makes sure sideEffects: true can clean up files
          preserveModules: true,
          output: {
            dir: "dist",
          },
        },
}

and it does output the correct file structure, but it requires some changes for everything else to still work correctly.

The main problem is that our entry files are named index.ts, which is the default for most people and tsdx. When enabling preserveModules it will then output index.js, overwriting the tsdx cjs entry file.

So for this to work you will also need to rename the entry files to something other than index.*, and set package.json's source property to them unless you want to pass --entry name.ts.

I ended up writing a common config file that also checks that the package.json has the correct configuration.

Re-usable tree-shaking config

// tsdx-configs.js
const { resolve } = require("path")
const { equal, notEqual } = require("assert")

const assertPkgPropertyEquals = (pkg, prop, expected) =>
  equal(pkg[prop], expected, `\`pkgJson.${prop}\` should be \`${expected}\``)

const assertPackageJsonIsCorrect = (dirname) => {
  const pkg = require(resolve(dirname, "package.json"))

  notEqual(
    pkg.source,
    null,
    "`pkgJson.source` field is missing, which is used as the entry point."
  )

  const [, filename] = /([\w-]+)\.\w+$/.exec(pkg.source) || []

  notEqual(
    filename,
    "index",
    "The `pkgJson.source` file cannot be named `index.*` due to tsdx output configuration."
  )

  assertPkgPropertyEquals(pkg, "main", "dist/index.js")
  assertPkgPropertyEquals(pkg, "module", `dist/${filename}.js`)
  assertPkgPropertyEquals(pkg, "types", `dist/${filename}.d.ts`)
}

module.exports = {
  treeShaking: (dirname) => ({
    rollup: (config, options) => {
      assertPackageJsonIsCorrect(dirname)

      return options.format !== "esm"
        ? config
        : {
            ...config,
            // this makes sure sideEffects: true can clean up files
            preserveModules: true,
            output: {
              dir: "dist",
            },
          }
    },
  }),
}

// packages/package/tsdx.config.js
const { treeShaking } = require("../../tsdx-configs")

module.exports = treeShaking(__dirname)

[agilgur5]

We don't need this for "deep imports"

I was responding to the subject of this issue. There is also #321 that doesn't have this specific use-case. Given that "deep imports" aren't what --preserveModules is for, that would make these duplicates, but in their current state they are not exactly.

I'll mark this as v0.15 milestone as well since they're basically duplicates now.

If we do something like import {A, B} from "pkg" and the package was built with preserveModules it will be able to tree-shake the rest of the code in the package, while if everything is bundled in one file webpack seems to be unable from our tests.

Can you provide a reproduction for this? I believe this would be helpful to the broader community as well.
I've seen many folks say that Rollup has the most advanced side-effect detection (I cannot verify this myself however), but if you have sideEffects: false, that means this ability shouldn't make a difference and Terser's DCE should, in theory, work just as well.

Immer also no longer uses preserveModules either, which is notable.

[agilgur5]

So I've experimented using Immer's config:

Ah, my bad I missed the output.dir part. I forgot preserveModules requires dir instead of file (#535 is 1.5 months old now, sorry). But Immer does not use that config currently, and I strongly suggest against it as it overrides the entire output object with a shallow merge. (also the comment is inverted)

The main problem is that our entry files are named index.ts, which is the default for most people and tsdx. When enabling preserveModules it will then output index.js, overwriting the tsdx cjs entry file.

Ah, sorry, yes, this is one of the issues in the current version of #535 . There I just specified output.dir as dist/esm/, but that requires some more changes to get working optimally; those are the current problems listed in #535.

But I actually made a better version with output.dir as plain dist/ in a later iteration of #367 that makes use of output.entryFileNames.

For usage here, the config would look like:

module.exports = {
  rollup(config, opts) {
    if (opts.format === 'esm') {
        config = { ...config, preserveModules: true }
        config.output = { ...config.output, dir: 'dist/', entryFileNames: '[name].esm.js' }
        delete config.output.file
    }
    return config
}

You'll have to change your package.json module to index.esm.js for this (unavoidable with output.dir) and there might still be some issues with how types are output. Might not be, but I'm not sure, #535 is still WIP after all and that's an issue I'm looking to resolve in a few places. EDIT: #691 should resolve any type output issues.

Have updated my previous code sample to this as well.

[beeequeue]

Can you provide a reproduction for this? I believe this would be helpful to the broader community as well.

I created an example reproduction using your better config.

Config-wise, it might be a better idea to name the ESM files [name].mjs to match Node's new ESM support

[beeequeue]

I've ran into another problem: if we use async/await and babel transpiles it we will get an invalid output due to babel-plugin-transform-async-to-promises/helpers not being external.

It ends up outputting the following structure:

packages/
  pkg/
    index.js
dist/
  node_modules/
    babel-plugin-transform-async-to-promises/
      helpers.mjs
  packages/
    pkg/
      index.mjs

This is a problem I encountered myself in the past and wasn't able to fix myself. :(

I guess we have to change how the plugin transforms async somehow?

EDIT: Seems like one option has to be set to false for it to work properly, but I don't seem to be able to override it with the .babelrc file The .babelrc file was in the wrong place.

[AlexanderGlusov]

I use the method described by a @agilgur5 for my ui-kit library but also with postcss plugin.

My tsdx.config.js

const postcss = require('rollup-plugin-postcss')
const autoprefixer = require('autoprefixer')
const cssnano = require('cssnano')

module.exports = {
  rollup(config, options) {
    if (options.format === 'esm') {
      config = { ...config, preserveModules: true }

      config.output = {
        ...config.output,
        dir: 'dist/',
        entryFileNames: '[name].esm.js',
      }

      delete config.output.file
    }

    config.plugins.push(
      postcss({
        modules: true,
        plugins: [
          autoprefixer(),
          cssnano({
            preset: 'default',
          }),
        ],
        inject: true,
        extract: false,
        minimize: true,
      })
    )

    return config
  },
}

But for some reason, the builded components in the '/dist' directory have incorrect imports, for example Button.esm.js has
import { objectWithoutPropertiesLoose as _objectWithoutPropertiesLoose } from '../../../../_virtual/_rollupPluginBabelHelpers.js'
instead of
import { objectWithoutPropertiesLoose as _objectWithoutPropertiesLoose } from '../../../_virtual/_rollupPluginBabelHelpers.js'

I understand that this may not be preserveModules issue, but maybe someone can give an advice