-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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
Better modularization of our plugins #22888
Conversation
What a blessed PR! Finally bootstrap is aligning with modern module systems! Can't wait to dig in, probably this weekend |
js/tests/visual/alert.html
Outdated
@@ -45,6 +45,7 @@ | |||
</div> | |||
|
|||
<script src="../../../assets/js/vendor/jquery-slim.min.js"></script> | |||
<script src="../../../assets/js/vendor/popper.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to remove 😆
Cool stuff! I'll be pretty offline for the finnish midsummer time, but I'd love to review this early next week! |
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 |
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 :
Switch to |
package.json
Outdated
@@ -57,7 +60,7 @@ | |||
}, | |||
"style": "dist/css/bootstrap.css", | |||
"sass": "scss/bootstrap.scss", | |||
"main": "dist/js/bootstrap", | |||
"main": "src/js/index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you were aiming for, but no need to change the main file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because with that entry point we can load only required plugins it allow folks to make small bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't point to the source but to a es5 transpiled version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way would be lazily evaluate the modules with import()
call, but not sure if that would work either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lazy load is overkill, better use named experts for each plugin/module so that people can import just what they need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I was thinking about the modularization part last night and my conclusions were something like this (if my idea of #22888 (comment) is correct):
|
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 : |
This is the same as |
Unfortunately when I did Edit : |
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 |
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. |
@FezVrasta There's nothing wrong with the code, as I stated above, I think |
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 With all your feedbacks a good beginning should be what we have currently, allow user to import only needed plugins with |
@petetnt the whole idea about named exports is that you can import the library and have three shake remove unused code super easily. Doing 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. |
@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. |
@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. |
js/src/index.js
Outdated
* -------------------------------------------------------------------------- | ||
*/ | ||
|
||
export default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @FezVrasta 👍
Related to this: 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. |
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 |
@addyosmani: do you have any idea how to solve this? Thanks in advance!
|
We, uh, have some conflicts here lol. |
Yep I'll take care of that 😆 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 yep I still have this issue, when we import a module every others modules are bundled too... If you have any idea about what I did wrong ? Or any other ideas it would be great, thank you 👍 |
external: external, | ||
globals: { | ||
jquery: '$', | ||
'popper.js': 'Popper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These dependencies should be defined as external, but not global. Bootstrap should actively try to import them (mostly to fix #23381)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because you don't have any import $ from 'jquery'
and import Popper from 'popper.js'
in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this we can do require(['jquery', 'popper.js', 'bootstrap'], ($, Popper, Bootstrap) => {})
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently I have this issue :
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope sorry, It's been a while since last time I used jQuery :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got to be sure but it seems that's because jQuery do not have an ES6 release 😟
Has anyone seen this from Webpack's docs here.
It seems tree shaking doesn't work on external packages yet. You would have to explicitly do |
Here they are using Rollup, not webpack |
Closing for now, I'll come back with a better PR, thanks to all you helped me a lot |
This PR will allow folks to import only modules they want to use for example :
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 finallyrollup
allow us to have smalls dist files