add Serverless V4 migration#708
Open
sid88in wants to merge 2 commits into
Open
Conversation
…: requires Serverless Framework v4; drops v3 support.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR upgrades serverless-appsync-plugin to Serverless Framework v4 by converting internal module imports to type-only, implementing a build-time deep-import validator, updating the e2e test harness for v4's authentication requirements, mocking the Serverless framework in unit tests, and documenting the new setup instructions. ChangesServerless v4 Migration
🎯 3 (Moderate) | ⏱️ ~22 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
27-28: ⚡ Quick winRun deep-import verification on freshly built artifacts.
verify:importsruns before compilation in this job, so it checks checked-inlib/only and can miss regressions introduced insrc/whenlib/is stale. Prefer runningnpm run buildhere (it already includesverify:imports).Suggested CI adjustment
- - name: Verify no deep serverless/lib imports (guards `#632`) - run: npm run verify:imports + - name: Build (includes deep-import guard) + run: npm run build🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 27 - 28, Replace the step that currently runs "npm run verify:imports" in the CI job named "Verify no deep serverless/lib imports (guards `#632`)" so it runs the full build instead; specifically, change the step to execute "npm run build" (which already runs verify:imports) to ensure deep-import checks run on freshly built artifacts rather than stale checked-in lib/.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 52-58: The e2e job currently always expects SERVERLESS_ACCESS_KEY
but pull_request workflows from forked repos don't have repo secrets; add a
job-level condition to skip/gate the e2e job when the PR is from a fork (e.g.,
use a condition based on github.event_name and
github.event.pull_request.head.repo.fork) so the job only runs when secrets are
available; update the job that defines env: SERVERLESS_ACCESS_KEY to include
that if-condition (or equivalent) to prevent hard-failing on forked PRs.
In `@e2e/helpers/synthesize.ts`:
- Around line 88-90: The code uses a fixed temporary config name (e2eConfigName
/ e2eConfigPath) which causes races when synth runs in parallel; change creation
to generate a unique filename (e.g., append process.pid + timestamp or a
random/UUID suffix or use fs.mkdtemp in os.tmpdir) when building
e2eConfigName/e2eConfigPath, update any cleanup logic to remove that same unique
path, and ensure fs.writeFileSync and any subsequent reads/packaging reference
the new unique variable so concurrent runs won't clobber each other.
In `@src/__tests__/commands.test.ts`:
- Around line 4-10: The test suite in src/__tests__/commands.test.ts currently
skips all domain command tests; restore coverage by either migrating the
existing fixture harness to Serverless v4 or adding v4-compatible smoke tests
that exercise the core command paths (domain create, domain delete, domain
assoc, domain disassoc, create-record, delete-record). Update or replace the
in-process fixture used in this file (and the other skipped suites referenced)
so it boots Serverless v4, or implement lightweight tests that invoke the plugin
command handlers directly (calling the command entrypoints used by the suite) to
assert basic success/failure behavior; ensure each restored test asserts the
expected result and runs in CI.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 27-28: Replace the step that currently runs "npm run
verify:imports" in the CI job named "Verify no deep serverless/lib imports
(guards `#632`)" so it runs the full build instead; specifically, change the step
to execute "npm run build" (which already runs verify:imports) to ensure
deep-import checks run on freshly built artifacts rather than stale checked-in
lib/.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 326f82ee-f089-4759-84c2-628cbb1ec346
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/ci.yml.gitignoree2e/README.mde2e/helpers/synthesize.tspackage.jsonscripts/check-no-serverless-deep-imports.jssrc/__tests__/commands.test.tssrc/__tests__/given.tssrc/index.ts
Comment on lines
+52
to
+58
| # Serverless Framework v4 requires authentication for every CLI invocation, | ||
| # including non-interactive CI. The e2e harness spawns `serverless package`, | ||
| # so a license/access key must be present or the run will fail/prompt. | ||
| # Add SERVERLESS_ACCESS_KEY (Dashboard > Settings > Access Keys) as a repo | ||
| # secret. Free for individual developers / orgs under the revenue threshold. | ||
| env: | ||
| SERVERLESS_ACCESS_KEY: ${{ secrets.SERVERLESS_ACCESS_KEY }} |
There was a problem hiding this comment.
Gate e2e on secret availability for fork PRs.
pull_request workflows from forks do not receive repository secrets, so this job will fail there by default. Add a job condition so fork PRs are not hard-failed by missing SERVERLESS_ACCESS_KEY.
Suggested job gate
e2e:
name: CFN Synthesis Tests
runs-on: ubuntu-latest
needs: tests
+ if: ${{ github.event_name != 'pull_request' || secrets.SERVERLESS_ACCESS_KEY != '' }}
# Serverless Framework v4 requires authentication for every CLI invocation,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 52 - 58, The e2e job currently always
expects SERVERLESS_ACCESS_KEY but pull_request workflows from forked repos don't
have repo secrets; add a job-level condition to skip/gate the e2e job when the
PR is from a fork (e.g., use a condition based on github.event_name and
github.event.pull_request.head.repo.fork) so the job only runs when secrets are
available; update the job that defines env: SERVERLESS_ACCESS_KEY to include
that if-condition (or equivalent) to prevent hard-failing on forked PRs.
Comment on lines
+88
to
+90
| const e2eConfigName = 'serverless.e2e.yml'; | ||
| const e2eConfigPath = path.join(absoluteExampleDir, e2eConfigName); | ||
| fs.writeFileSync( |
There was a problem hiding this comment.
Avoid shared temporary config filename across parallel synth runs.
Line 88 uses a fixed serverless.e2e.yml; concurrent synth calls for the same example can overwrite/delete each other’s config and make packaging flaky.
Suggested fix
- const e2eConfigName = 'serverless.e2e.yml';
+ const e2eConfigName = `serverless.e2e.${process.pid}.${Date.now()}-${Math.random()
+ .toString(36)
+ .slice(2)}.yml`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const e2eConfigName = 'serverless.e2e.yml'; | |
| const e2eConfigPath = path.join(absoluteExampleDir, e2eConfigName); | |
| fs.writeFileSync( | |
| const e2eConfigName = `serverless.e2e.${process.pid}.${Date.now()}-${Math.random() | |
| .toString(36) | |
| .slice(2)}.yml`; | |
| const e2eConfigPath = path.join(absoluteExampleDir, e2eConfigName); | |
| fs.writeFileSync( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/helpers/synthesize.ts` around lines 88 - 90, The code uses a fixed
temporary config name (e2eConfigName / e2eConfigPath) which causes races when
synth runs in parallel; change creation to generate a unique filename (e.g.,
append process.pid + timestamp or a random/UUID suffix or use fs.mkdtemp in
os.tmpdir) when building e2eConfigName/e2eConfigPath, update any cleanup logic
to remove that same unique path, and ensure fs.writeFileSync and any subsequent
reads/packaging reference the new unique variable so concurrent runs won't
clobber each other.
Comment on lines
+4
to
+10
| // NOTE: This suite is skipped pending a Serverless v4 test-harness migration. | ||
| // It drives the plugin's domain commands through `@serverless/test`'s in-process | ||
| // fixture engine (see ./utils), which boots the framework from | ||
| // `node_modules/serverless`. Serverless v4 ships only a thin installer there (the | ||
| // framework runs from a downloaded binary), so this v3-era engine cannot load it. | ||
| // Re-homing these command tests onto a v4-compatible harness is tracked as | ||
| // follow-up work. Local stand-in keeps the file compiling while skipped. |
There was a problem hiding this comment.
Domain command coverage is fully disabled.
All domain command suites are skipped, so regressions in domain create/delete/assoc/disassoc/create-record/delete-record can ship undetected. Please keep v4-compatible coverage for these command paths before release (full harness migration or at least smoke coverage per command group).
Also applies to: 30-30, 230-230, 321-321, 540-540, 693-693, 912-912
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/commands.test.ts` around lines 4 - 10, The test suite in
src/__tests__/commands.test.ts currently skips all domain command tests; restore
coverage by either migrating the existing fixture harness to Serverless v4 or
adding v4-compatible smoke tests that exercise the core command paths (domain
create, domain delete, domain assoc, domain disassoc, create-record,
delete-record). Update or replace the in-process fixture used in this file (and
the other skipped suites referenced) so it boots Serverless v4, or implement
lightweight tests that invoke the plugin command handlers directly (calling the
command entrypoints used by the suite) to assert basic success/failure behavior;
ensure each restored test asserts the expected result and runs in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CR: Migrate to Serverless Framework v4 (drop v3) —
v3.0.0Closes #632 (and upstream dup serverless/serverless#12565:
Cannot find module 'serverless/lib/serverless-error'on Serverless v4).Type: breaking change — drops Serverless v3 support, requires Serverless v4.
Suggested title:
feat!: require Serverless Framework v4, drop v3 supportWhy
Serverless v4 ships the
serverlessnpm package as a thin installer with nolib/directory — the framework internals moved into a bundled@serverless/framework. So any runtimerequire('serverless/lib/...')against av4 install throws
MODULE_NOT_FOUND, which is exactly #632.Two findings from auditing the plugin against v4's actual source shaped this CR:
compiled
require('serverless/lib/serverless-error')inlib/resources/Api.js(reporters were on
^2.7.x). Current code routes errors throughthis.serverless.classes.Error, and the emittedlib/has zeroserverless/librequires.new Plugin(serverless, options, utils)(3rd-arglog/progress/writeText),serverless.classes.Error,getProvider('aws'),provider.request,configSchemaHandler.defineTopLevelProperty,configurationVariablesSources,addServiceOutputSection,service.setFunctionNames,processedInput/configurationInput, theaws:infolifecycle, andfinalize.It also auto-honors the community override when
serverless-appsync-pluginislisted in
plugins:.So this CR is not a runtime rewrite. It (a) hardens the code so #632 can never
silently return, (b) flips the version contract to v4, and (c) re-points the e2e
harness/CI at v4. Runtime-behavioral confirmation on v4 is the one piece this
change can't self-verify — see Maintainer-gated steps below.
What changed
src/index.tsserverless/lib/Serverless,…/provider.js) and the type imports fromserverlessare nowimport type— fully erased at compile time, so they can never become runtime requires. Added a comment explaining the #632 link.scripts/check-no-serverless-deep-imports.jslib/**/*.jsand fails the build if anyrequire('serverless/lib/...')appears. Anti-regression for #632.package.jsonbuildnow runs the guard (tsc && npm run verify:imports); newverify:importsscript.peerDependencies.serverless→^4.0.0. devserverless→^4.0.0..github/workflows/ci.ymlSERVERLESS_ACCESS_KEY(v4 requires auth for every CLI invocation, including non-deploypackage).e2e/helpers/synthesize.tscloudformation-template-update-stack.json, falls back to anycloudformation-template-*-stack.json— so the harness survives any v4 path nuance.e2e/README.mdengines.nodeis left at>=20(v4 requires>=18.17; CI matrix is 20/22/24/26).Verification done in this change
npm run build— clean; the guard runs and reports no deep imports.npm run lint(eslint .+tsc --noEmit, coverse2e/) — clean.npm test— 288 passing, 210 snapshots, 18 suites (unchanged).lib/**/*.jscontains zeroserverless/librequires.0on clean output, exit1when arequire('serverless/lib/serverless-error')is injected.Maintainer-gated steps (cannot run without a v4 license + AWS context)
These are the steps that require your environment — a Serverless Access/License
key and the v4 binary — and are the real acceptance gate:
npm install(updatespackage-lock.jsonforserverless@4). The bumpeddevDependencieswill not be reflected in the lockuntil this runs, so
npm cijobs will fail until the regenerated lock iscommitted.
SERVERLESS_ACCESS_KEY(Dashboard → Settings →Access Keys) under repo Settings → Secrets and variables → Actions.
export SERVERLESS_ACCESS_KEY=… && npm run test:e2e.Triage any assertion failures as genuine v3↔v4 CFN output diffs (most assertions
target AppSync resources we emit, so the majority should hold). Confirm v4 still
emits the template where the (now-resilient) harness expects.
Breaking changes
peerDependencies.serverless: ^4.0.0).Serverless v3 is no longer supported and is itself unmaintained upstream as of
early 2025.
serverlessto v4 and authenticate the v4 CLI.Release
Versioning here is tag-driven (
release.ymlsets the version from the tag). Shipthis as
v3.0.0by pushing thev3.0.0tag once merged.Out of scope (deliberate follow-ups)
Dependency modernization is intentionally not in this CR to keep the v4-enablement
diff small and reviewable. Suggested as a separate
v3.1.0:aws-sdk@^2(EOL) →@aws-sdk/client-*(v3) — currently used type-only insrc.@serverless/utilsruntime dep (not imported anywhere insrc).Reviewer checklist
import typeis correct for the deep imports (verify they're type-only usages).SERVERLESS_ACCESS_KEYsecret added before merging (else e2e job fails).package-lock.jsonregenerated and committed.Summary by CodeRabbit
Chores
Documentation
Refactor