Fix: JS tree shaking

[astagi]

Description

This PR aims to solve tree shaking issues as described in #37575 issue.

Motivation & Context

I created a little demo here with a detailed README to show the problem I'm experiencing with the current version of Bootstrap and a possible solution using a patched version of the library. Basically I can't get tree shake working on my project unless I change the way you create the esm file (single index.esm.js and then a single file for each component, e.g. src/alert.js, src/tooltip.js)

Type of changes

• [X] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Refactoring (non-breaking change)
• [ ] Breaking change (fix or feature that would change existing functionality)

Checklist

• [X] I have read the contributing guidelines
• [X] My code follows the code style of the project (using npm run lint)
• [ ] My change introduces changes to the documentation
• [ ] I have updated the documentation accordingly
• [ ] I have added tests to cover my changes
• [ ] All new and existing tests passed

Live previews

• https://deploy-preview-{your_pr_number}--twbs-bootstrap.netlify.app/

Related issues

Fixes #37575

[julien-deramond]

Thanks for this PR @astagi. We haven't looked at it yet but could you please remove the modifications in dist/js? It's going to be generated/built when we'll make the release.

[XhmikosR]

Chances are this is not needed at all. We ship index.esm.js after 5.3.0 alpha 1, doesn't this work for you?

On Tue, Dec 27, 2022, 19:18 Julien Déramond @.***> wrote:

Thanks for this PR @astagi https://github.com/astagi. We haven't looked at it yet but could you please remove the modifications in dist/js? It's going to be generated/built when we'll make the release.

— Reply to this email directly, view it on GitHub https://github.com/twbs/bootstrap/pull/37733#issuecomment-1366056649, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVLNJ7TV5T4AWH3KMXFO3WPMQE3ANCNFSM6AAAAAATKSRPXA . You are receiving this because your review was requested.Message ID: @.***>

[astagi]

Thanks for this PR @astagi. We haven't looked at it yet but could you please remove the modifications in dist/js? It's going to be generated/built when we'll make the release.

Sure @julien-deramond just removed and forced update.

@XhmikosR do you mean bootstrap.esm.js? I think is not needed, that's why I removed it from build scripts. The remaining structure will be

dist
- js
-- index.esm.js
-- src
---- alert.js
---- tooltip.js

[XhmikosR]

I don't think we need another JS dist file. For package users, using src/index.esm.js should be enough. Have you tried it with specify bootstrap/js/src/index.esm.js?

When the ESM files were introduced, we only targeted browser users and we shouldn't introduce any breaking changes.

[astagi]

I don't think we need another JS dist file. For package users, using src/index.esm.js should be enough. Have you tried it with specify bootstrap/js/src/index.esm.js?

When the ESM files were introduced, we only targeted browser users and we shouldn't introduce any breaking changes.

@XhmikosR haven't tried using src/index.esm.js directly, I usually compile everything using Rollup and its plugins and put generated files and maps in the dist folder. I agree with you, in this case it's the same files structure you have in src.. I don't know if using src/index.esm.js directly is the correct way, in that case src/index.esm.js file should be referenced in package.json as module alongside the sideEffects (this property actually activates tree shaking).

I'm curious to try modules in browsers, I usually bundle everything in a single js files using modules from packages, anyway I don't think things would change: imho using a single esm file with class declarations and code to initialize components can't be optimized in any way.

[astagi]

@XhmikosR another thing that concerns me: using src/index.esm.js directly as a module would skip all the Rollup plugins chain such as Babel.. any idea on this?

[astagi]

@XhmikosR @julien-deramond I made some tests (here with my version supporting tree shaking and here with the current Bootstrap version)! I will restore it alongside index.esm.js file that should correctly be referenced in package.json alongside sideEffects to make tree shaking work using Webpack or Rollup. The only thing I needed to do is using an import map for Popper.js

<script type="importmap">
    {
        "imports": {
            "@popperjs/core": "./node_modules/@popperjs/core/dist/esm/index.js"
        }
    }
</script>

Do you know why I need this?

By my side I'll restore old ESM build asap.

[astagi]

@XhmikosR @julien-deramond I've just force updated this branch, I made some tests and as @XhmikosR suggested, using only "js/index.esm.js" as module everything works as expected (Luxon library uses this approach as well). Now we have

• dist/bootstrap.esm.js (and minified version) for browser usage.
• js/index.esm.js as module for Webpack, Rollup etc.. with sideEffect set to false to enable tree shaking.

Here you can find my demo updated with this configuration to see things in action! Same results, no breaking changes, no file touched/added in dist folder 🎉

[XhmikosR]

Unfortunately, it is still a breaking change because the now referenced file is not passed through babel.

That's why I only decided to include the js/index.esm.js for people to manually reference it when they build Bootstrap. For browser-ready builds only the dist file can be used like before.

We need to revisit our bundles in v6 and see what we are shipping. And also, remove the dist files from the repo and later see if we can split our CSS and JS into separate packages.

[astagi]

Unfortunately, it is still a breaking change because the now referenced file is not passed through babel.

That's why I only decided to include the js/index.esm.js for people to manually reference it when they build Bootstrap. For browser-ready builds only the dist file can be used like before.

We need to revisit our bundles in v6 and see what we are shipping. And also, remove the dist files from the repo and later see if we can split our CSS and JS into separate packages.

Sorry @XhmikosR what do you mean with "the now referenced file"? Index.esm.js? It doesn't need to be passed through Babel, is something that should be used with bundlers like Rollup or Webpack and is up to users to make it pass through Babel for the final bundle in their project configuration.. this is the same approach used by Luxon and it's the correct way.. the old bootstrap.esm.js that passes through Babel is the file that can be used in browsers, if you reference this file inside module it won't be tree shaked and this will impact on the final bundle size..

Let me also add that I don't know how many users are using that module directly in browsers, I followed the documentation to use Bootstrap with Rollup.js using import syntax, that's the only thing I found about modules and following the documentation I discovered that tree shaking was missing.

[astagi]

@XhmikosR @julien-deramond I can confirm that I don't have any breaking with my modification. I tried using the module in a html page

    <script async type="module">
        import { Alert, Tooltip } from 'bootstrap'

        const alert = new Alert(document.getElementsByClassName('alert')[0])

        setTimeout(() => {
            alert.close()
        }, 5000);

        document.querySelectorAll('.ttp').forEach(el => {
            new Tooltip(el);
        });
    </script>

I'm forced to use an importmap to make it working and here I can reference bootstrap.esm.js (note: usually imports will reference files from node_modules, here I use a relative import with a bootstrap version containing my commits).

    <script type="importmap">
       {
           "imports": {
               "@popperjs/core": "/bootstrap/node_modules/@popperjs/core/dist/esm/index.js",
               "bootstrap": "/bootstrap/dist/js/bootstrap.esm.js"
           }
       }
   </script>

Please see the example here. js/index.esm.js referenced in package.json is only used by bundlers.

[XhmikosR]

The change in package.json is the breaking change. Before the file was passed through Babel, now it isn't.

[astagi]

The change in package.json is the breaking change. Before the file was passed through Babel, now it isn't.

@XhmikosR Yes I know. With my modification index.esm.js is not passed through Babel because it's intended to be used as module by users.. users can do *import {Button} from "bootstrap" * in their projects and then use it as a module (parsing with Babel using their specific configuration, bundle it and minify). The module section in package.json is not intended to be used by browsers but by Node.js, imho there's no need to make it pass through Babel (it will be done later by the user in their projects if they need).

On the other side bootstrap.esm.js (that is still present and not touched by this modification) is intended to be used by browsers, and people must define a pathmap or include it directly to use it inside browsers (and this happened before my modification as well) despite the file defined in package.json.

Please see Luxon source code linked above. Sorry for these long posts and thanks for your patience and your replies, but I don't understand what's wrong with this PR. I'd like to understand to improve bootstrap-italia project as well.