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

DO NOT MERGE maintenance: bundle miniflare #342

Closed
wants to merge 1 commit into from

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Jan 30, 2022

This bundles miniflare into the wrangler package. We already bundled most other dependencies, this adds miniflare as well. Of note, it's actually bundling miniflare twice, once into the main bundle, and once separately as a cli to be called with wrangler dev as local mode. We could optimise this in the future as a separate package. Regardless, I expect this to anyway install fewer dependencies (naturally, because it's not a nested dep anymore) and roughly be the same size for download, so it should be a faster install.

I don't think this should break anything else. This would've been a good usecase for the arbitrary npm publishes, haha. Anyway, we can test the alpha after we land this.

Closes #66

@changeset-bot
Copy link

changeset-bot bot commented Jan 30, 2022

🦋 Changeset detected

Latest commit: ce2a2dd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@threepointone threepointone changed the title WIP maintenance: bundle miniflare maintenance: bundle miniflare Jan 31, 2022
@threepointone threepointone marked this pull request as ready for review January 31, 2022 09:09
@petebacondarwin
Copy link
Contributor

Just for information...

Before this PR:

=== Tarball Details === 
name:          wrangler                                
version:       0.0.15                                  
filename:      wrangler-0.0.15.tgz                     
package size:  2.4 MB                                  
unpacked size: 12.7 MB                                 
total files:   88 

After this PR:

=== Tarball Details === 
name:          wrangler                                
version:       0.0.15                                  
filename:      wrangler-0.0.15.tgz                     
package size:  4.1 MB                                  
unpacked size: 20.0 MB                                 
total files:   86

@@ -1,6 +1,7 @@
.DS_Store
node_modules/
wrangler-dist/
miniflare-dist/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this so that the outputs all go into dist? E.g.

dist
  wrangler
    cli.js
    cli.js.map
  miniflare
    cli.js

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we only need to ignore dist whatever else we decide to put in there.

@petebacondarwin
Copy link
Contributor

More interesting info...
I tried using the npm packed version of wrangler from main and this PR.
The result of npm install:

main

37,456,739 bytes (39.1 MB on disk) for 808 items

This PR

38,472,480 bytes (38.9 MB on disk) for 194 items

So the size on disk is almost identical. But you can see that for this PR we download about a quarter as many items.
Also the package-lock.json indicates many many fewer dependencies (as suggested by @threepointone in the PR description).
Finally, "ad hoc" install time measurement went from 14 secs to 7 secs 🎉

@petebacondarwin
Copy link
Contributor

So all-in-all, this looks like a good improvement.

I think we should definitely consider bundling the miniflare library separately from the miniflare CLI, so we can reduce the duplication there (see the size increase in the bundle, above). Or alternatively work out a way to kick off miniflare for dev mode without having to use the miniflare CLI...?

@threepointone
Copy link
Contributor Author

threepointone commented Jan 31, 2022

Unfortunately, calling the CLI is necessary so we can attach a debugger :( That's also why we're manually invoking the cli with node --inspect instead of invoking miniflare's bin. Maybe the thing to do here is for pages, and our kv commands etc to use the bundled cli instead, which would remove all instances of import {Miniflare} from 'miniflare' from the source. I'll leave that for a subsequent PR.

@mrbbot
Copy link
Contributor

mrbbot commented Jan 31, 2022

Hey! 👋 This is a good idea. However, I looked into this and remember having a few issues with undici (can't remember exactly just some unhandled rejections I think). html-rewriter-wasm also expects a .wasm file in the same directory as the package, so you'd probably need to copy that over too. I'm pretty swamped with work at the moment so won't be able to look into this.

@threepointone
Copy link
Contributor Author

Thanks for the headsup! Aight, I'll make sure I handle html-rewriter-wasm correctly.

@threepointone
Copy link
Contributor Author

I think a simple fix should be to just mark html-rewriter-wasm as an external, and add it as a dependency. Do you remember how to trigger the undici issue?

@mrbbot
Copy link
Contributor

mrbbot commented Jan 31, 2022

I not too sure, it either happened every time when returning a Response or when using fetch?

@threepointone
Copy link
Contributor Author

Ok thank you @mrbbot, I'm able to reproduce the issue. I'll park this PR until I fix it. Thanks for the assist, I appreciate you!

@threepointone threepointone changed the title maintenance: bundle miniflare DO NOT MERGE maintenance: bundle miniflare Jan 31, 2022
@threepointone threepointone marked this pull request as draft January 31, 2022 11:01
This bundles miniflare into the wrangler package. We already bundled most other dependencies, this adds miniflare as well. Of note, it's actually bundling miniflare _twice_, once into the main bundle, and once separately as a cli to be called with `wrangler dev` as local mode. We could optimise this in the future as a separate package. Regardless, I expect this to anyway install fewer dependencies (naturally, because it's not a nested dep anymore) and roughly be the same size for download, so it should be a faster install.
@petebacondarwin
Copy link
Contributor

This PR has got a bit stale and is not crucial to getting to 2.0.0.
Let's close it for the time-being and revisit after we release 2.0.0.

@threepointone threepointone mentioned this pull request Apr 28, 2022
6 tasks
@JacobMGEvans JacobMGEvans deleted the bundle-miniflare branch August 15, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generate a compiled version of miniflare
3 participants