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

Add find_additional_modules option to support partial bundling with externals #3726

Merged
merged 23 commits into from
Oct 5, 2023

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Aug 9, 2023

Disclaimer - WIP and untested!

This fairly chunky branch contains code that should allow us to search for rules matching modules when also bundling Workers.

Hey! 👋 This should be good to go now, see #3726 (comment) for details.

@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2023

🦋 Changeset detected

Latest commit: cce3d79

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

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

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6418496031/npm-package-wrangler-3726

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6418496031/npm-package-wrangler-3726

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6418496031/npm-package-wrangler-3726 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6418496031/npm-package-cloudflare-pages-shared-3726

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.10.1 includes the following runtime dependencies:

Package Constraint Resolved
miniflare 3.20231002.0 3.20231002.0
workerd 1.20231002.0 1.20231002.0
workerd --version 1.20231002.0 2023-10-02

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #3726 (cce3d79) into main (5fc7a88) will increase coverage by 0.23%.
The diff coverage is 86.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3726      +/-   ##
==========================================
+ Coverage   75.09%   75.32%   +0.23%     
==========================================
  Files         217      222       +5     
  Lines       12077    12179     +102     
  Branches     3128     3149      +21     
==========================================
+ Hits         9069     9174     +105     
+ Misses       3008     3005       -3     
Files Coverage Δ
packages/wrangler/src/api/pages/deploy.tsx 86.15% <ø> (ø)
packages/wrangler/src/config/validation.ts 90.03% <ø> (ø)
packages/wrangler/src/deploy/deploy.ts 87.90% <100.00%> (+0.31%) ⬆️
...wrangler/src/deployment-bundle/apply-middleware.ts 100.00% <100.00%> (ø)
...s/wrangler/src/deployment-bundle/build-failures.ts 100.00% <100.00%> (ø)
...ages/wrangler/src/deployment-bundle/bundle-type.ts 100.00% <100.00%> (ø)
...src/deployment-bundle/create-worker-upload-form.ts 89.85% <100.00%> (+1.05%) ⬆️
...s/wrangler/src/deployment-bundle/dedupe-modules.ts 100.00% <100.00%> (ø)
packages/wrangler/src/deployment-bundle/rules.ts 100.00% <100.00%> (ø)
packages/wrangler/src/dev.tsx 83.53% <ø> (ø)
... and 17 more

... and 3 files with indirect coverage changes

@mrbbot
Copy link
Contributor

mrbbot commented Aug 18, 2023

Hey! 👋 I think this PR is ready to go now (minus some docs). You can now add something like...

find_additional_modules = true
rules = [
  { type = "ESModule", globs = ["**/*.js"]},
]

...to your wrangler.toml file, and have Wrangler bundle your entrypoint as usual, but upload all *.js files as is, marking them external. Appropriate files are watched in wrangler dev to trigger rebuilds when files are changed/created.

This gives us a slightly nicer "no bundle" mode, with the ability to use middleware whilst maintaining lazy loading/dynamic imports of --no-bundle mode.

Note, find_additional_modules is disabled when using service workers as it relies on the ability to pass multiple modules to the runtime, which isn't supported with service worker scripts.

Closes #3074

@mrbbot mrbbot marked this pull request as ready for review August 18, 2023 17:34
@mrbbot mrbbot requested review from a team as code owners August 18, 2023 17:34
@mrbbot mrbbot changed the title WIP: Bundle refactoring Add find_additional_modules option to support partial bundling with externals Aug 18, 2023
@mrbbot mrbbot requested a review from RamIdeas August 18, 2023 17:35
@mrbbot
Copy link
Contributor

mrbbot commented Aug 21, 2023

Blocked by cloudflare/miniflare#631

@dario-piotrowicz
Copy link
Member

Confirming that this works and it's making the new lazy loading implementation of the workers nuxt adapter work 😃 🚀 🎉

See https://nuxt-split-workers-test.devdash.workers.dev/ (source)

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Look good to me 😃

I gave the PR a somewhat thorough read and it looks all good to me, but I am very very unfamiliar with the codebase so this for sure still needs to be reviewed by someone much much more familiar with it than me

Note
This requires #3799 in order to work

packages/wrangler/src/deployment-bundle/build-failures.ts Outdated Show resolved Hide resolved
packages/wrangler/src/deployment-bundle/build-failures.ts Outdated Show resolved Hide resolved
@petebacondarwin petebacondarwin force-pushed the bundle-refactoring branch 4 times, most recently from 73f2b7d to 00a8590 Compare August 31, 2023 18:54
@mrbbot mrbbot mentioned this pull request Sep 5, 2023
2 tasks
@petebacondarwin petebacondarwin force-pushed the bundle-refactoring branch 2 times, most recently from 07919ca to d095eff Compare September 6, 2023 10:19
petebacondarwin and others added 22 commits October 5, 2023 12:36
The no-bundle-import tests would actually pass if Wrangler actually bundled the code,
since it would identify and inline all the dynamic
imports that were being tested.

This change adds a test that would not pass if we are not capturing the additional modules.

It also moves the declaration of no-bundle to the wrangler.toml to ensure that normal use of `wrangler dev` will get this behavior.
This has a few of benefits:
- we don't risk creating too many file reads in a single go (using up all the available file handles); previously file reads were all done in parallel
- we don't read the files over and over for each rule
- the code is a bit easier to follow as we don't have to create `Promise.all()` objects
When using `find_additional_modules` (or `no_bundle`) we find files that
will be uploaded to be deployed alongside the Worker.

Previously, if an `outDir` was specified, only the Worker code was output
to this directory. Now all additional modules are also output there too.
@petebacondarwin petebacondarwin merged commit 7d20bdb into main Oct 5, 2023
23 checks passed
@petebacondarwin petebacondarwin deleted the bundle-refactoring branch October 5, 2023 13:25
@workers-devprod workers-devprod mentioned this pull request Oct 5, 2023
@irvinebroque
Copy link
Contributor

Is there context on why we added this but didn't put in dev docs?

@petebacondarwin
Copy link
Contributor Author

Is there context on why we added this but didn't put in dev docs?

An oversight on my part, or at least I got pulled onto other stuff and it fell off my radar; on top of that it was mainly designed for advanced users who kind of know the code already. But I think it is worth documenting this now.

@petebacondarwin
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants