Skip to content
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

Request: New dist/controller.min.js that only includes Controller. #638

Closed
dkniffin opened this issue Jan 17, 2023 · 13 comments
Closed

Request: New dist/controller.min.js that only includes Controller. #638

dkniffin opened this issue Jan 17, 2023 · 13 comments

Comments

@dkniffin
Copy link
Contributor

dkniffin commented Jan 17, 2023

Over on https://discuss.hotwired.dev/t/advice-request-stimulus-webpack-best-practices-bundle-splitting-for-multi-page-ssr-site/612/12 we're discussing how to split apart stimulus controllers into separate packs in webpack. I think we've landed on a good way to do that:

// app/javascript/packs/globals.js
import { Application } from "@hotwired/stimulus";
window.Stimulus = Application.start();
// app/javascript/packs/foo_controller.js
import FooController from "controllers/foo_controller"
application.register("foo", FooController)
// app/javascript/packs/bar_controller.js
import BarController from "controllers/bar_controller"
application.register("bar", BarController)

This works. However, it means that Stimulus is loaded into each pack, increasing the size of each pack unnecessarily. The only thing we need from Stimulus in there is Controller.

To fix this, I want to request creating a new file dist/controller.min.js so that we can do import { Controller } from '@hotwired/stimulus/dist/controller.js'

@dkniffin
Copy link
Contributor Author

I'm able to workaround this by adding window.Controller = Controller to global.js, but that pollutes the global namespace, and I'd rather not do that.

@marcoroth
Copy link
Member

Hey @dkniffin, I just checked how that might work. Technically this is possible, but I'm wondering if it wouldn't make more sense if you instead tree-shake the bundle on your end.

Technically the tool should be smart enough to pull out the Constant and extract the related "dependencies" with it. Or is there any reason why you wouldn't want to go that route?

@dkniffin
Copy link
Contributor Author

@marcoroth maybe my app is configured wrong, but it's not tree-shaking everything else out currently. Do you know if there's a config for that in webpacker?

@marcoroth
Copy link
Member

marcoroth commented Jan 18, 2023

@dkniffin
Copy link
Contributor Author

@marcoroth I took a look at that. I'll need to dig in more to my config and see if something is misconfigured. I'm definitely doing import { Controller } from "@hotwired/stimulus" so tree-shaking should work, I think.

But if I understand the webpack options correctly, only usedExports would shake out the unnecessary parts, right? I'm guessing sideEffects won't work, because all of Stimulus is in one file/module in the minified version. I suppose I could try unminified, then minify it as part of my build process, but that means I'd be (unnecessarily) building Stimulus every time.

@marcoroth
Copy link
Member

@dkniffin the tree-shaking should be smart enough to figure out which parts to keep even if the sources are minified. The sideEffects option is meant for files in your project which are not side-effect free.

@dkniffin
Copy link
Contributor Author

@marcoroth I found out that my project was not configured correctly for tree-shaking. I tested by adding all of lodash as an unused export and checking the bundle size via webpack-bundle-analyzer. It was including everything.

It turns out I was missing "sideEffects": false from my package.json

HOWEVER, once I got that enabled, it did not make a difference on the size of Stimulus inside my packs that only use Controller. So it seems Stimulus can't be tree-shaken correctly, I think? It's also possible it has something to do with my pack splitting. 🤔 I'll dig into that more tomorrow.

@marcoroth
Copy link
Member

marcoroth commented Jan 31, 2023

It would be interesting to see if this would also help for your case here: #623 (comment)

If you would do something like:

import { Controller } from "@hotwired/stimulus/dist/core/controller"

@dkniffin
Copy link
Contributor Author

@marcoroth yep! That made a huge difference.

So, tree shaking isn't working with the full module, but it does work with the individual file. So I think my original request here is still valid. Is that something you'd consider including in the build?

@dkniffin
Copy link
Contributor Author

@marcoroth actually, I just looked into the "sideEffects": false and it turns out that doesn't work. It certainly reduces the bundle size, but it also removes all the stimulus controllers from it 😬 So that won't work.

I think somewhere in my configs, tree-shaking is not properly configured. But I'm not sure where. I tested by adding a function in one of my own files with a ton of lorem-ipsum text. Then I import another function from that file. I'd expect that function to be removed, but it's not. I don't understand.

Wepback(er)... 😩

But I did test with the dev build and import you suggested, and without "sideEffects": true, and that still helps my problem.

@marcoroth
Copy link
Member

Using #686 you should now be able to import the controller class like:

import { Controller } from "@hotwired/stimulus/dist/core/controller"

The tree-shake config depends on the bundler you are using. But with having the classes available by themselves should allow any bundler to tree-shake the files and imports accordingly.

Feel free to re-open this issue if you still think we need something else in Stimulus. Thank you!

@markhealey
Copy link

In version 3.2.2 this file does not exist...

@marcoroth
Copy link
Member

@markhealey they will ship with the next version of Stimulus. In the meantime, you can install the latest dev-build to already make use of them today:

yarn add @hotwired/stimulus@https://github.com/hotwired/dev-builds/archive/refs/tags/@hotwired/stimulus/349dc16.tar.gz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants