Better modularization of our plugins #22888

Johann-S · 2017-06-22T13:24:24Z

This PR will allow folks to import only modules they want to use for example :

import { Util, Modal } from 'bootstrap'

This PR introduce the use of rollup and add two new dist files :

bootstrap.bundle.js
bootstrap.bundle.min.js

This two files include Popper.js so when you include those files in your HTML pages you don't have to include Popper.js before Bootstrap.

/CC @mdo @petetnt @bardiharborow @hnrch02
Close: #19017, #22783

Edit :
/CC @IdanCo if they are something to change in our webpack get started example
I know the branch of this PR is webpack but finally rollup allow us to have smalls dist files

IdanCo · 2017-06-22T13:33:02Z

What a blessed PR! Finally bootstrap is aligning with modern module systems! Can't wait to dig in, probably this weekend

Johann-S · 2017-06-22T13:45:46Z

js/tests/visual/alert.html

@@ -45,6 +45,7 @@
 </div>

 <script src="../../../assets/js/vendor/jquery-slim.min.js"></script>
+ <script src="../../../assets/js/vendor/popper.min.js"></script>


Something to remove 😆

petetnt · 2017-06-22T20:21:21Z

Cool stuff! I'll be pretty offline for the finnish midsummer time, but I'd love to review this early next week!

FezVrasta · 2017-06-22T23:14:40Z

I'd suggest to go with Rollup instead of Webpack since it's best suited for libraries bundling (the webpack maintainers think the same).

This would also reduce the bundle size

Johann-S · 2017-06-23T05:46:33Z

I'll give another try to Rollup in another branch and if I have something something better I'll incorporate that here thanks @FezVrasta 👍

Edit :
First try with Rollup for our dist/js/bootstrap.js :

With Webpack : 132 kb,
With Rollup : 106 kb

Switch to rollup done 👍

IdanCo · 2017-06-25T19:34:01Z

package.json

@@ -57,7 +60,7 @@
 },
 "style": "dist/css/bootstrap.css",
 "sass": "scss/bootstrap.scss",
- "main": "dist/js/bootstrap",
+ "main": "src/js/index",


not sure what you were aiming for, but no need to change the main file

Because with that entry point we can load only required plugins it allow folks to make small bundle

You shouldn't point to the source but to a es5 transpiled version

I agree with @FezVrasta (and in any case i think you meant js/src and not src/js)

Nevertheless, there is still some issue with importing specific modules (it happens with either main file) . I havn't figured out the root cause, but the symptom is that both import "bootstrap" and import { module } from "bootstrap" ends up loading ALL modules.

To test it i've created a new branch of an empty scaffold project, and added @Johann-S latest commit as a dependency. Feel free to clone and do npm install. Then npm run serve will run the app with the new dependency. notice that you should manually fix "package.json" in the bootstrap directory to point to the right file (dist/js/bootstrap) before serving.

And on a side note - great job @Johann-S ! With this new build, importing bootstrap with webpack (or any other bundler for that matter) becomes a simple one liner. Sadly it will render the entire installation guide i wrote obsolete, but i'll cope ;-)

Sorry I forgot to mention that the Es5 + es modules version should be defined in the "module" property, while the es5 w/o es modules (umd usually) should point to "main"

Re: ends up loading ALL modules issue is documented in my original issue #19017. Re-exported modules will call all the IIFEs when the index file is evaluated, leading into namespace pollution. See https://codesandbox.io/s/vLXO14lg for an example.

In my branch I opted in for having module users to initialize their own components with ModuleNameHere.init(), while the browser bundles (including single modules, like tooltip.js) auto-initialize themselves with a secret variable ([1] [2]) which solves the problem but creates some cognitive load for the end user.

Another way would be lazily evaluate the modules with import() call, but not sure if that would work either.

Lazy load is overkill, better use named experts for each plugin/module so that people can import just what they need

That's only possible by importing the modules with import Tooltip from 'bootstrap/js/some-module-name'. Importing the modules anywhere (like in index.js) evaluates the files as evident in https://codesandbox.io/s/Mjgm6qzAm, rendering import { SomeModule } from 'bootstrap' pretty moot as it's always equivalent of import Bootstrap from 'bootstrap'.

Userland could adapt some bundler that's smart enough to tree-shake the other modules but even that might be a scretch. Do correct me if I am wrong though.

petetnt · 2017-06-26T06:17:52Z

I was thinking about the modularization part last night and my conclusions were something like this (if my idea of #22888 (comment) is correct):

Without any escape hatch for preventing auto-init importing all the modules essentially boils down to import 'bootstrap' as the modules don't really have that many static methods available that would be useful outside of the modules themselves.
Importing import { Tooltip, Modal } from 'bootstrap' most likely is not going to happen without any side-effects...
however, releasing the modules as namespaced versions (for example @bootstrap/tooltip, @bootstrap/modal on npm would provide modularity for those who want to include only parts of the modules themselves (which is actually pretty nice, as proven for example by https://lodash.com/ and its namespaced modules (https://www.npmjs.com/browse/keyword/lodash-modularized).
one could also import the modules from the folders with import 'bootstrap/js/tooltip' or import 'bootstrap/js/modal'.

Johann-S · 2017-06-26T08:50:07Z

I have some tests to do with all your comments thanks @petetnt, @FezVrasta and @IdanCo

I'm not sure if it's the best way but something like this works very well to import only needed plugins :
import Modal from 'bootstrap/js/dist/modal'

FezVrasta · 2017-06-26T08:55:15Z

This is the same as import { Modal} from 'bootstrap', but the latter works better and follows the conventions

Johann-S · 2017-06-26T08:57:31Z

Unfortunately when I did import { Modal } from 'bootstrap' @FezVrasta every other plugins were imported and not only Modal plugin

Edit :
@petetnt that's right what did lodash here : https://github.com/lodash/lodash/tree/npm-packages allow to export plugins separately, but it seems pretty hard to maintain a separate branch only for our js plugins

FezVrasta · 2017-06-26T09:10:19Z

This means there's something wrong with your code. I'd say you may want to merge this PR just to provide the needed tooling. Then we may work together on future PRs to improve the code splitting.

I may even work on this personally once this PR is merged

IdanCo · 2017-06-26T09:12:01Z

I think you're in the right direction. IMO, loading the entire library should remain as simple as possible. importing individual modules is usually the doing of more advanced users, in which case bootstrap can allow itself a bit more complex implementation.

In any case i'll do some testings later today to see if it's possible to hold the stick on both ends by fixing the "all or nothing" issue.

petetnt · 2017-06-26T09:15:56Z

@FezVrasta There's nothing wrong with the code, as I stated above, import { Modal } from 'bootstrap' is not equivalent of import Modal from 'bootstrap/js/dist/modal'. It's just that the index.js file is evaluated when import * from 'bootstrap' is called like I explained before.

I think lodash is now built with lodash-cli/ nowadays but building 9 individual files with RollUp and creating a simple release script would not be too time consuming, it can be done later too though.

Johann-S · 2017-06-26T09:27:13Z

I wont merge this PR untily Beta 1 is shipped, but you can submit a PR which target this branch @FezVrasta 👍

Thanks @IdanCo, do not hesitate to let us know what result you had

@petetnt with this PR rollup build seperately each of our files with build/build-plugins.js script but yes it's a good idea to create a release script for that 👍

With all your feedbacks a good beginning should be what we have currently, allow user to import only needed plugins with import { <plugin name> } from 'bootstrap/js/dist/<plugin name> and for folks who needs the entire libs allow us to do import { <plugin name> } from 'bootstrap' or simply import 'bootstrap'

FezVrasta · 2017-06-26T09:53:58Z

@petetnt the whole idea about named exports is that you can import the library and have three shake remove unused code super easily.

Doing import Modal from 'bootstrap/js/dist/modal' is just a way to import a single file, but it doesn't take advantage of the ES Modules architecture and relies on internal structure of the library, which may change in the future and break others code.

To test that the library properly uses static imports and ES Modules you should import it in a dummy project with wepkack or Rollup, both of them support three shake, and the outputted bundle should include only what you actually imported.

petetnt · 2017-06-26T10:45:48Z

@FezVrasta I already mentioned userland treeshaking in #22888 (comment). The issue is that will tree shakers eliminate the IIFEs that pollute global namespace, especially when it's done against the already-once built files (when things like Webpack 2 have had problems with treeshaking and re-exports in general: webpack/webpack#2867).

If it works, then fine, but the side-effect is still unwanted and unexpected in general.

FezVrasta · 2017-06-26T11:21:17Z

@petetnt I think we should look forward and comply with the modern standards (in this case, named exports + three shaking) and don't stagnate on old habits.

In this case, IIFE and UMD aren't bundle formats that ever considered three shaking as part of them, and nobody is trying to make them work with it.
We should provide ability to three shake just to who actually uses a tooling capable to handle it properly.

Johann-S · 2017-06-26T12:15:36Z

js/src/index.js

+ * --------------------------------------------------------------------------
+ */
+
+export default {


Currently I export our plugins with export default but I'm not sure if this is the good way to have allow folks to :
import { Util, Modal } from 'bootstrap' ?

Nope, if you export default { stuff } you are exporting an object with inside the modules.

If you want to allow users to import single modules, you have to do export { stuff1, stuff2, etc }

Thanks @FezVrasta 👍

FezVrasta · 2017-06-26T12:18:29Z

Related to this:
#22888 (comment)

Nothing prevent us from providing both the solutions, if people use modern toolings, they will want to use the ES5 + ESM bundle to take advantage of three shaking.

If they use old tooling, they'll want to use the UMD/IIFE bundle, and, in this case, we may provide sub-bundles with the specific modules inside.

Johann-S · 2017-06-26T12:25:55Z

I agree with @FezVrasta, if I try to allow folks to only import specific plugins it's to allow them to have small bundle when they import bootstrap in their projects so tree-shaking is an important thing to concider here

XhmikosR · 2017-07-16T08:49:37Z

@addyosmani: do you have any idea how to solve this? Thanks in advance!

To sum up if someone trys to build a project with Bootstrap and just want to include our Util.js with import { Util } from 'bootstrap' currently the other source files are added too... They isn't any tree shaking done.

mdo · 2017-08-11T06:22:06Z

We, uh, have some conflicts here lol.

Johann-S · 2017-08-11T06:24:21Z

Yep I'll take care of that 😆
About that PR I can't go further here, everything is done, if someone have other feedbacks don't hesitate 👍

Currently it seems it's a bit hard for modules bundler to tree shake huge library, I hope in a near futur it'll be the case 🙏

addyosmani · 2017-08-12T21:39:04Z

@XhmikosR Just saw this. Sorry for the delayed response.

@Johann-S Do you know to what extent your latest commits using RollUp are failing to tree-shake as expected? Do you still see the problem of other modules (e.g beyond the 'Util' example) getting pulled in?

Johann-S · 2017-08-13T06:34:31Z

@addyosmani yep I still have this issue, when we import a module every others modules are bundled too...
Maybe that's because Bootstrap depends a lot of jQuery.
I read a few days the Troubleshooting section of Rollupjs (see : https://github.com/rollup/rollup/wiki/Troubleshooting#tree-shaking-doesnt-seem-to-be-working) and it indicate that if a library is too complex Rollup won't tree shake because they don't want to break everything.

If you have any idea about what I did wrong ? Or any other ideas it would be great, thank you 👍

FezVrasta · 2017-08-18T13:23:54Z

build/rollup.config.js

+ external: external,
+ globals: {
+ jquery: '$',
+ 'popper.js': 'Popper'


These dependencies should be defined as external, but not global. Bootstrap should actively try to import them (mostly to fix #23381)

if I remove globals I have those warnings :

I think it's because you don't have any import $ from 'jquery' and import Popper from 'popper.js' in the code?

That's wrong in this PR we import Popper.js and jQuery (for example see : https://github.com/twbs/bootstrap/blob/webpack/js/src/tooltip.js)

Sorry, probably you still need those for the UMD and IIFE bundles, but you must make sure no never reference to them in the source, but always require them with import

If someone want to use Bootstrap in a UMD module a rollupjs config like this would work perfectly without : window.Popper = Popper :

With this we can do require(['jquery', 'popper.js', 'bootstrap'], ($, Popper, Bootstrap) => {})?

Currently I have this issue :
⚠️ 'jquery' is imported by ..\bootstrap\js\dist\esm\scrollspy.js, but could not be resolved – treating it as an external dependency

So I'm not sure in an UMD build everything works too...

EDIT :
My ESM files import jQuery and that's why I have this issue 😩

RE EDIT :
jQuery is difficult to import... For now I cannot make it work without something like this :
<script src="https://code.jquery.com/jquery-3.2.1.slim.min.js"></script>
But for Popper.js it works perfectly

Do you have any ideas @FezVrasta ?

Nope sorry, It's been a while since last time I used jQuery :/

I've got to be sure but it seems that's because jQuery do not have an ES6 release 😟

prograhammer · 2017-08-30T21:15:42Z

Has anyone seen this from Webpack's docs here.

To get most out of tree shaking with external packages, you have to use babel-plugin-transform-imports to rewrite imports so that they work with webpack's tree shaking logic. See webpack issue #2867 for more information.

It seems tree shaking doesn't work on external packages yet. You would have to explicitly do import Bar from 'foo/bar'.

FezVrasta · 2017-08-30T21:29:48Z

Here they are using Rollup, not webpack

Johann-S · 2017-10-06T08:40:41Z

Closing for now, I'll come back with a better PR, thanks to all you helped me a lot

Johann-S added js meta v4 labels Jun 22, 2017

Johann-S mentioned this pull request Jun 22, 2017

Bootstrap JavaScript modules API design questions/issues #19017

Closed

Johann-S commented Jun 22, 2017

View reviewed changes

mdo requested review from bardiharborow, fat and cvrebert June 22, 2017 15:30

Johann-S force-pushed the webpack branch from 683c5a0 to 8166313 Compare June 25, 2017 07:07

IdanCo reviewed Jun 25, 2017

View reviewed changes

Johann-S commented Jun 26, 2017

View reviewed changes

Johann-S force-pushed the webpack branch from e7e798e to ccbd3c8 Compare June 26, 2017 12:35

Johann-S mentioned this pull request Jul 17, 2017

Treat jQuery & Tether as dependencies in ES6 source code #17201

Closed

This was referenced Jul 31, 2017

Missing semicolon at the end of bootstrap.js causes issues when bundled with other scripts #23175

Closed

Remove jquery from required packages #23204

Closed

Johann-S force-pushed the webpack branch from a52032f to 4860f1c Compare August 9, 2017 11:17

Johann-S force-pushed the webpack branch from 4860f1c to a042354 Compare August 11, 2017 11:29

Johann-S mentioned this pull request Aug 17, 2017

Bootstrap dropdown require Popper.js (https://popper.js.org) #23381

Closed

FezVrasta reviewed Aug 18, 2017

View reviewed changes

Johann-S added 9 commits August 18, 2017 16:31

Build our plugins with Rollup

45aed43

Add header to our dist js files with Rollup

1f4f451

Update npm-shrinkwrap.json

7b905d1

Fix build bootstrap.bundle.js

41c4695

Fix main entry point, add module entry point, and build index.js

4b35521

Change index.js module name to Bootstrap because it's the entry point

7a8d417

Export named plugins

b701da2

Init our plugins only on browser and when DOM is ready

c23cffe

Build our plugins in ESM

248dfcb

Johann-S requested a review from XhmikosR August 18, 2017 15:14

Johann-S force-pushed the webpack branch from a042354 to 248dfcb Compare August 18, 2017 15:16

Johann-S mentioned this pull request Aug 19, 2017

PR: Add support for module property in package.json #23566

Closed

Johann-S closed this Oct 6, 2017

mdo deleted the webpack branch May 1, 2018 03:57

Better modularization of our plugins #22888

Better modularization of our plugins #22888

Conversation

Johann-S commented Jun 22, 2017 • edited Loading

IdanCo commented Jun 22, 2017

Choose a reason for hiding this comment

petetnt commented Jun 22, 2017

FezVrasta commented Jun 22, 2017

Johann-S commented Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petetnt Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petetnt commented Jun 26, 2017

Johann-S commented Jun 26, 2017 • edited Loading

FezVrasta commented Jun 26, 2017

Johann-S commented Jun 26, 2017 • edited Loading

FezVrasta commented Jun 26, 2017 • edited Loading

IdanCo commented Jun 26, 2017

petetnt commented Jun 26, 2017 • edited by Johann-S Loading

Johann-S commented Jun 26, 2017

FezVrasta commented Jun 26, 2017 • edited Loading

petetnt commented Jun 26, 2017

FezVrasta commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FezVrasta commented Jun 26, 2017

Johann-S commented Jun 26, 2017

XhmikosR commented Jul 16, 2017

mdo commented Aug 11, 2017

Johann-S commented Aug 11, 2017 • edited Loading

addyosmani commented Aug 12, 2017

Johann-S commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johann-S Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prograhammer commented Aug 30, 2017 • edited Loading

FezVrasta commented Aug 30, 2017

Johann-S commented Oct 6, 2017

Johann-S commented Jun 22, 2017 •

edited

Loading

Johann-S commented Jun 23, 2017 •

edited

Loading

petetnt Jun 25, 2017 •

edited

Loading

Johann-S commented Jun 26, 2017 •

edited

Loading

Johann-S commented Jun 26, 2017 •

edited

Loading

FezVrasta commented Jun 26, 2017 •

edited

Loading

petetnt commented Jun 26, 2017 •

edited by Johann-S

Loading

FezVrasta commented Jun 26, 2017 •

edited

Loading

Johann-S commented Aug 11, 2017 •

edited

Loading

Johann-S Aug 18, 2017 •

edited

Loading

prograhammer commented Aug 30, 2017 •

edited

Loading