🚀 Feature Request: Make bundling and rules 2 different concepts

vicb commented 1 month ago

1) We have rules to upload different files:

rules = [
  { type = "CommonJS", globs = ["**/*.js"], fallthrough = true }
]

2) By default wrangler bundles all the files together (using esbuild) that is rules from 1 above are ignore and all the imports are inlined.

As of today 1 and 2 are grouped around a single "Bundling" paradigm.

That is:

wrangler deploys will activate 2 and ignore 1
wrangler deploys --no-bundle will activate 1 and skip 1 (esbuild)

I believe 1 and 2 should be different concepts and not mutually exclusive.

For example you could have you framework composed of multiple files and then a directory containing the different pages of your application.

- app/
  - index.js
  - util.js
  - render.js
- pages/
  - homepage.js  
  - about.js

In that case it would be nice to bundle the app into a single file (2) and being able to require the page matching the url (1).

Updating ESBuild could solve some of the use case with wildcard imports.

workerd recently introduced createRequire. You can now write:

import { createRequire } from 'module';
const require = createRequire('/');
const msg = require('./hello.cjs').world();
console.log(`hello ${msg}`);

The code works fine with --no-bundle - as long as the hello.cjs has a matching rule in wrangler.toml. However the code will fail without --no-bundle because it can not resolve hello.cjs. If require is passed a variable, the code will fail to build because of the dynamic require.

That's why I believe we should be able to bundle the app/framework but still have module that you can require.

If both 1 and 2 can be used together, files matched by a rules must be marked as external so that esbuild does not try to inline them.

/cc @IgorMinar

threepointone commented 1 month ago

Can you explain the usecase you're going for?

vicb commented 1 month ago

@threepointone

The code snippet I added in the description above would not work consistently wether bundling is turned on or not:

import { createRequire } from 'module';
const require = createRequire('/');
const msg = require('./hello.cjs').world();
console.log(`hello ${msg}`);

Also if at some point we lazy parse module on worked, it could save CPU time

threepointone commented 1 month ago

right, got it. can you try by adding find_additional_modules = true in your wrangler.toml?

vicb commented 1 month ago

It looks the the (undocumented feature) I was looking for, thanks! I'll give it a try.

vicb commented 1 month ago

find_additional_modules = true seems to work - I am not exactly sure how.

It looks like we have "implicit" rules which will always add [.html, .txt, ...](find_additional_modules = true) as additional modules - I don't think there is a way to disable those?
find_additional_modules = true seems to apply to wrangler dev but has not effect for wrangler dev --no-bundle

Q1) Is my understanding right? Q2) what does "additional" refers to in "find_additional_modules"? Does it mean in addition to the implicit rules? Q3) Would it make sense that wrangler dev and wrangler dev --no-bundle have the same behavior (that is respect the rules) by default - I think it would Q4) Would it make sense for rules to have a "Typescript" type ? (The rationale here is that we would compile the main, why not do the same for additional modules).

I'm asking all those question to understand and try to update the docs. Thanks

edit: because my interepretation of "bundling" was not the right one. To me it means bundling the main entry point.

/cc @petebacondarwin

petebacondarwin commented 1 month ago

find_additional_modules will cause Wrangler to traverse the directories below the base_dir (which defaults to the directory containing the main entry-point) looking for files ("modules") to include in the Worker upload in "addition" to the entry-point module.

It only includes modules (files) that match rules as specified in your wrangler.toml (plus the default rules).

Rules are evaluated per "type" and if you set fallthrough to be false then it will not try subsequent rules for that type, which gives you some control over turning off the default rules (by providing impossible globs). (I think)

When no_bundle is true, then find_additional_modules is true by default. So I am surprised that you found there was different behaviour.

We don't do any processing at all of the "additional" modules; which is a gap in the tooling, I agree. Not sure if we need another type though because it would also be true that you might want to process JavaScript.

Finally, the Vite integration might also provide an alternative approach to all of this. Watch this space...

petebacondarwin commented 1 month ago

Also, bundling is an over used term in this field. We often need distinguish between the single file that esbuild generates (the "esbuild bundle") and the collection of modules that are uploaded to Miniflare/Cloudflare (the "deployment bundle").

irvinebroque commented 1 month ago

Super supportive of us clarifying in docs, agree this is confusing today and lot of opportunity to get super clear about current behavior. Would love us to do that first before adding new stuff in this area, so that we're building off of a base that there's a shared understanding of.

vicb commented 1 week ago

Pete has documented find_additional_modules in https://github.com/cloudflare/cloudflare-docs/pull/17524 (mayeb we should add the implicit rules for JS files).

vicb commented 2 days ago

I'd like to propose to drop find_additional_modules for wrangler v4.

Current documentation for find_additional_modules:

(Approximate) behavior of find_additional_modules:

You can defined rules in your wrangler configuration to attach additional modules to your worker script.

Those rules are taken into account only when no_bundle is true.

When no_bundle is false (the default value) then the rules are ignored unless find_additional_modules is explicitly set to true. It seems that setting find_additional_modules to true also add implicit rules - possibly this and this?

"(Approximate)" in the section title is because of those implicit rules that are not documented.

Why I think we should drop find_additional_modules:

Defining rules should be enough. There should be no need to have an additional and confusing find_additional_modules.

As of today whether or not rules are taken into account depends on the values of no_bundle and find_additional_modules - this is not well documented.

Also it looks like enabling find_additional_modules will enable implicit rules that are not documented.

How to migrate:

I believe we can drop find_additional_modules and always look for modules matching the user defined rules when they exist.

In the migration docs, we should also explicit the implicit rules for users to add them to their rules.

To migrate from find_additional_modules: true in v3, users would remove find_additional_modules from their v4 config and add the implicit rules to their list of rules.

petebacondarwin commented 2 days ago

Those rules are taken into account only when no_bundle is true.

This is not quite accurate. When no_bundle is false these rules still play a part:

When esbuild comes across an import that matches one of the rules it marks it as external and ensures that this file appears in the bundle.
The reason it appears that it is not working is that Wrangler is hard coded to ignore .js rules 😱 See https://github.com/cloudflare/workers-sdk/blob/675c2f0055be8a9c6f7432f4a1d0bfdcb55e5d9d/packages/wrangler/src/deployment-bundle/module-collection.ts#L256-L260

petebacondarwin commented 2 days ago

I wonder if actually what we should do is allow the user to specify .js rules whether or not find_additional_modules is turned on? This is a breaking change that we could do in v4.

vicb commented 2 days ago

When no_bundle is false these rules still play a part:

Only if find_additional_modules is set to true, right? (That's what I meant by "When no_bundle is false (the default value) then the rules are ignored unless find_additional_modules is explicitly set to true")

petebacondarwin commented 2 days ago

When no_bundle is false these rules still play a part:

Only if find_additional_modules is set to true, right? (That's what I meant by "When no_bundle is false (the default value) then the rules are ignored unless find_additional_modules is explicitly set to true")

No I don't think that is the case. For example you can have a WASM rule that ensures that imported WASM files end up in the bundle.

petebacondarwin commented 2 days ago

See this example: https://developers.cloudflare.com/workers/wrangler/configuration/#importing-modules-within-a-worker That should work with no_bundle = false and find_additional_modules = false.

vicb commented 2 days ago

I really can not make sense of find_additional_modules, may be it's related to the bug you mentioned in https://github.com/cloudflare/workers-sdk/issues/6944#issuecomment-2498422487. I'll look at in the morning tomorrow :)

cloudflare / workers-sdk

🚀 Feature Request: Make bundling and rules 2 different concepts #6944