Repository navigation
[deploy-helpers] Expose dedicated workflow entry points - #16624
petebacondarwin wants to merge 9 commits into
Conversation
🦋 Changeset detectedLatest commit: b6c8d98 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const helpers = await import("@cloudflare/deploy-helpers"); | ||
| for (const [entry, name] of [["deploy", "deploy"], ["versions-upload", "versionsUpload"], ["triggers", "triggersDeploy"], ["preview", "previewBuildOutput"]]) { | ||
| const workflow = await import("@cloudflare/deploy-helpers/" + entry); | ||
| assert.equal(workflow[name], helpers[name]); | ||
| } |
There was a problem hiding this comment.
🔍 Import guard misses cross-workflow loading
The root import loads every workflow before the dedicated entries run. These checks verify export identity and runtime avoidance, but cannot detect whether a dedicated entry loads other workflows.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
LGTM |
![clanky-the-clanker[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
a049c6d5.
This review, of 60785cb9
💬 Approved with comments — The entry points are wired up correctly and tested. The only finding is an INFO about a comment in tsup.config.ts that describes too many entry points as sharing the context.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
A tsup config comment describes too many entry points as sharing the context
Recommendation: 💬 Optional Nits
- [INFO] The tsup.config.ts comment says all entry points share context.ts, but create-worker-upload-form and startup-profile don't import it. Narrow the comment to the entry points that use the context. (
packages/deploy-helpers/tsup.config.ts:7)
View full review
The comment at packages/deploy-helpers/tsup.config.ts:7-10 says all entry points share context.ts. create-worker-upload-form and startup-profile do not import it. The comment would be more accurate if it said that the entry points which use the context share its singleton.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset covers the new entry points, and the existing root exports are unchanged.
🔒 Security: ✅ LGTM
No security concerns in these entry-point and packaging changes.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository's guidance.
📋 Code Quality: ✅ LGTM
The new src/deploy/index.ts, src/triggers/index.ts and src/preview/index.ts re-export existing modules. The subpath exports in package.json and the tsup.config.ts entries match. The dry-run test's assertion matches triggersDeploy's behaviour when it is called with dryRun: true and validated: true.
⚡ Performance: ✅ LGTM
No performance concerns. Each focused entry point lets a command import its own workflow without loading the package's other workflows.
Reviewed commit: 60785cb9
…-helpers-workflow-entries # Conflicts: # packages/deploy-helpers/tests/package-imports.test.ts
There was a problem hiding this comment.
Superseded by the review of
82ece831.
This review, of a049c6d5
💬 Approved with comments — The entry points are wired up correctly and tested. The only open finding is an INFO: a comment and a README sentence say all entry points share the context, when only the workflow entry points do.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
The tsup comment and the README say all entry points share the context, but only the workflow entry points do.
Recommendation: 💬 Optional Nits
- [INFO] The tsup.config.ts comment (and README.md:16-17) says all entry points share context.ts, but create-worker-upload-form and startup-profile don't import it. Limit the wording to the entry points that use the context. (
packages/deploy-helpers/tsup.config.ts:7)
View full review
packages/deploy-helpers/tsup.config.ts:7says "All entry points share context.ts as a singleton".packages/deploy-helpers/README.md:16-17says "All entry points share the same context". Neither thecreate-worker-upload-formnor thestartup-profileentry point imports the context. Limit both statements to the entry points that use it.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset documents the new entry points, and the existing root exports are unchanged.
🔒 Security: ✅ LGTM
No security concerns in these entry-point and packaging changes.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository's guidance.
📋 Code Quality: ✅ LGTM
The package.json subpath exports match the tsup.config.ts entry names. src/deploy/index.ts, src/triggers/index.ts and src/preview/index.ts re-export the same modules as src/index.ts, which is why the identity checks in the test hold. The conflict resolution in package-imports.test.ts is consistent with the rest of the diff.
⚡ Performance: ✅ LGTM
No performance concerns. With the dedicated entry points, a command can import its own workflow without loading the package's other workflows.
Reviewed commit: a049c6d5
There was a problem hiding this comment.
Superseded by the review of
2a5ccace.
This review, of 82ece831
✅ Approved — No new findings. The subpath exports match the build entries, and the identity and import-guard tests cover the new workflow entry points. One earlier wording note on the shared-context comment is left to the author.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset documents the new entry points as a minor bump, which matches the repository's policy for new features. The existing root exports are unchanged.
🔒 Security: ✅ LGTM
No security concerns in these entry-point and packaging changes.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository's guidance.
📋 Code Quality: ✅ LGTM
The package.json subpath exports match the entry names in tsup.config.ts. The new entry-point modules re-export the same modules as src/index.ts, so the identity checks in the test hold. The updated built-package guard in package-imports.test.ts matches the rest of the diff.
⚡ Performance: ✅ LGTM
No performance concerns. With the dedicated entry points, a command can import its own workflow without loading the package's other workflows.
📝 Documentation: ✅ LGTM
No new documentation issues. The earlier note about the "all entry points share the context" wording at tsup.config.ts:7 and README.md:16-17 is still open and left to the author.
Reviewed commit: 82ece831
…-helpers-workflow-entries # Conflicts: # packages/deploy-helpers/tests/package-imports.test.ts
There was a problem hiding this comment.
Superseded by the review of
27b2b251.
This review, of 2a5ccace
✅ Approved — No new findings. The subpath exports match the build entries, and the identity and import-guard tests cover the new workflow entry points. One earlier wording note on the shared-context comment is left to the author.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset documents the new entry points as a minor bump, which matches the repository's policy for new features. The existing root exports are unchanged.
🔒 Security: ✅ LGTM
No security concerns in these entry-point and packaging changes.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository's guidance.
📋 Code Quality: ✅ LGTM
The package.json subpath exports match the entry names in tsup.config.ts. The new entry-point modules re-export the same modules as src/index.ts, so the identity checks in the test hold. The updated built-package guard in package-imports.test.ts matches the rest of the diff.
⚡ Performance: ✅ LGTM
No performance concerns. With the dedicated entry points, a command can import its own workflow without loading the package's other workflows.
📝 Documentation: ✅ LGTM
No new documentation issues. The earlier note about the "all entry points share the context" wording at tsup.config.ts:7 and README.md:16-17 is still open and left to the author.
Reviewed commit: 2a5ccace — same changes as 82ece831, so this is that commit's review (no new review was run; line numbers are as of 82ece831)
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/runtime-types
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
…-helpers-workflow-entries
There was a problem hiding this comment.
Superseded by the review of
b6c8d985.
This review, of 27b2b251
✅ Approved — No new findings. The subpath exports match the build entries, and the identity and import-guard tests cover the new workflow entry points. One earlier wording note on the shared-context comment is left to the author.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset documents the new entry points as a minor bump, which matches the repository's policy for new features. The existing root exports are unchanged.
🔒 Security: ✅ LGTM
No security concerns in these entry-point and packaging changes.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository's guidance.
📋 Code Quality: ✅ LGTM
The package.json subpath exports match the entry names in tsup.config.ts. The new entry-point modules re-export the same modules as src/index.ts, so the identity checks in the test hold. The updated built-package guard in package-imports.test.ts matches the rest of the diff.
⚡ Performance: ✅ LGTM
No performance concerns. With the dedicated entry points, a command can import its own workflow without loading the package's other workflows.
📝 Documentation: ✅ LGTM
No new documentation issues. The earlier note about the "all entry points share the context" wording at tsup.config.ts:7 and README.md:16-17 is still open and left to the author.
Reviewed commit: 27b2b251 — same changes as 82ece831, so this is that commit's review (no new review was run; line numbers are as of 82ece831)
…-helpers-workflow-entries
There was a problem hiding this comment.
✅ Approved — No new findings. The subpath exports match the build entries, and the identity and import-guard tests cover the new workflow entry points. One earlier wording note on the shared-context comment is left to the author.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset documents the new entry points as a minor bump, which matches the repository's policy for new features. The existing root exports are unchanged.
🔒 Security: ✅ LGTM
No security concerns in these entry-point and packaging changes.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository's guidance.
📋 Code Quality: ✅ LGTM
The package.json subpath exports match the entry names in tsup.config.ts. The new entry-point modules re-export the same modules as src/index.ts, so the identity checks in the test hold. The updated built-package guard in package-imports.test.ts matches the rest of the diff.
⚡ Performance: ✅ LGTM
No performance concerns. With the dedicated entry points, a command can import its own workflow without loading the package's other workflows.
📝 Documentation: ✅ LGTM
No new documentation issues. The earlier note about the "all entry points share the context" wording at tsup.config.ts:7 and README.md:16-17 is still open and left to the author.
Reviewed commit: b6c8d985 — same changes as 82ece831, so this is that commit's review (no new review was run; line numbers are as of 82ece831)
Relates to cloudflare/cf#390. Stacked on #16623; merge that PR first.
Expose
@cloudflare/deploy-helpers/deploy,/versions-upload,/triggers, and/previewso CLI commands can import their workflow without pulling in the package's other workflows. Existing root exports remain available, and all entry points share the context initialized through/context.With the shared workers-utils helpers and Miniflare v5 export removal from #16623, each workflow entry point can be imported without loading Miniflare's runtime or resolving workerd. Startup profiling remains behind the existing
/startup-profileentry point and the lazy upload-error diagnostics path.Validation after refreshing the parent PR: all 297 deploy-helpers tests pass, including built-package import guards that reject Miniflare's main entry and workerd, entry-point identity checks, and a trigger dry run using the shared context. Package builds, source/test type checks, type-aware lint, and formatting pass. Full root
pnpm checkpasses after an unfilteredpnpm install --frozen-lockfile: all 204 tasks succeeded. The earlier missingtinyglobbyfailure was a local filtered-install omission and required no repository change.