[create-cloudflare] Enable upload_source_maps by default for new projects#12741
Conversation
🦋 Changeset detectedLatest commit: 67f5869 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 |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
✅ All changesets look good |
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: |
c5dc447 to
f9c48fa
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
/bonk fix the formatting in this pr |
There was a problem hiding this comment.
Looks good. The implementation is clean and follows the established patterns well.
What I verified:
- Correctness: Both TOML (
??=) and JSON (appendJSONProperty→assign) paths are consistent with howobservabilityandnodejs_compatare already handled. The??=for TOML won't override if a template already sets the value;assignfor JSON will override, but no C3 templates currently setupload_source_maps, so this is a non-issue in practice. - Python exclusion: Correctly grouped inside the
lang !== "python"guard alongsidenodejs_compat, which makes sense since Python Workers don't produce JS source maps. - Tests: Both TOML and JSON Python tests assert
upload_source_mapsis absent. All existing snapshot tests were updated to includeupload_source_maps = true. All 219 tests pass. - Changeset: Minor bump for
create-cloudflare, description is clear and well-written.
One minor nit below about the doc comment, feel free to take or leave it.
|
Minor nit: The doc comment for for completeness alongside the existing items for |
|
I've reviewed and approved PR #12741. Here's a summary of my findings: Approved - The implementation is clean and follows established patterns. Key points verified:
One minor nit posted as a comment: the doc comment on |
f9c48fa to
ead9aa8
Compare
…ects Add upload_source_maps: true to the central updateWranglerConfig function so all C3 templates get source maps enabled without needing per-template changes. Skipped for Python projects. Co-Authored-By: smacleod <samuel@macleod.space>
ead9aa8 to
67f5869
Compare
Supersedes #11726.
Instead of adding
upload_source_maps: trueto each of the 43 individual templatewrangler.jsoncfiles, this sets it centrally in theupdateWranglerConfigfunction — the same place that already setsname,compatibility_date,observability, andnodejs_compatfor all C3 templates.Like
nodejs_compat,upload_source_mapsis skipped for Python projects.Key review points
??=is used (won't override if already set). For JSON configs,appendJSONProperty(which callsassign) is used (will override). This matches the existing pattern forobservabilityvsnodejs_compatbut is worth verifying is intentional.upload_source_mapsis grouped withnodejs_compatinside thelang !== "python"guard. The original PR Set upload_source_maps to true for all C3 templates #11726 also removed it from Python templates explicitly.Link to Devin session: https://app.devin.ai/sessions/9aa9987751da4a5b8ed3cf97a28996f5
Devin PR requested by @penalosa