-
Notifications
You must be signed in to change notification settings - Fork 1k
Configurable rules for exclusions in bundling of python_modules #10927
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 93c0915 The changes in this PR will be included in the next version bump. 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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
63ae386
to
466f746
Compare
466f746
to
fbade0e
Compare
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.
Looks very reasonable to me. Thanks @dom96!
23af335
to
dd3cd86
Compare
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 am chiming in there because rules
/ find_additional_modules
is something that is very hard to grasp for the users (and for me TBH).
I'm afraid that adding one more config that is Python specific would make things even more confusing.
Because we already have rules
, couldn't we have a concept of exlcude rules, i.e. adding {exclude: true}
and it would apply for any language?
That's a fair point and I agree that all these options are challenging to understand. I'd love to reuse Our These modules are in some ways special, as they should always be implicitly included when present and wrangler should avoid printing out the full list of them (we have code that condenses these in the "Attaching additional modules" output). So I think from that perspective allowing users to exclude certain modules explicitly makes sense. But maybe you have another idea of what might work better? |
Another alternative is to do what was implemented here #10929, though that behaviour is a lot more subtle |
dd3cd86
to
9aaf849
Compare
9aaf849
to
93c0915
Compare
Discussed this with @petebacondarwin and we would prefer that Python specific configuration has its own section in the config rather than being mixed together with other top-level properties. How about the following? {
"python": {
"include": [],
"exclude": []
}
} You can just have |
Or |
Sounds good, I'll add a new |
Implements a new
python_modules_excludes
wrangler config field. The user can specify a list of globs in that field to exclude them from being vendored. By default every file inpython_modules
is included but for certain packages they contain so much stuff that sometimes it's good to be able to filter it out, especially since our bundle sizes are limited.Test Plan