fix: remove egg module#438
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRemoved Changes
Sequence Diagram(s)sequenceDiagram
participant App as "ServiceWorkerApp"
participant Loader as "LoadUnitLifecycle"
participant Hook as "LoadUnitInnerClassHook"
participant Creator as "EggPrototypeCreatorFactory"
participant Registry as "EggPrototypeFactory"
participant Runner as "ServiceWorkerRunner"
participant Context as "Request Context"
App->>Loader: register LoadUnitInnerClassHook
Note over App,Hook: Hook configured with inner classes
Loader->>Hook: postCreate(loadUnit)
alt loadUnit.type == StandaloneLoadUnitType
Hook->>Creator: createProto(clazz, loadUnit)
Creator-->>Hook: proto (async)
Hook->>Registry: registerPrototype(proto, loadUnit)
end
Runner->>Context: ContextHandler.getContext()
Context-->>Runner: request-local context with FetchEvent
Runner->>Runner: validate fetch event, lookup handler by event.type
Runner->>FetchEventHandler: dispatch fetch handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the ServiceWorkerRunner class and its export, deletes the eggModule configuration, and downgrades the package version from 3.78.7 to 3.78.3. The removal of ServiceWorkerRunner is identified as a critical issue because ServiceWorkerApp relies on it to dispatch events. Furthermore, the version downgrade in package.json is flagged as a likely mistake that could cause dependency resolution issues.
I am having trouble creating individual review comments. Click here to see my feedback.
standalone/service-worker/src/ServiceWorkerRunner.ts (1-16)
Removing the ServiceWorkerRunner class will break the core functionality of ServiceWorkerApp. The Runner instance initialized in ServiceWorkerApp (line 76) and executed in handleEvent (line 91) relies on finding a class decorated with @Runner() within the load units to act as the entry point. Without this runner, there is no logic to dispatch incoming events to their respective handlers (such as FetchEventHandler).
standalone/service-worker/package.json (4)
The version is being downgraded from 3.78.7 to 3.78.3. This is likely a mistake as package versions should typically be incremented to ensure proper dependency resolution and consistency with other packages in the repository (which are currently at 3.78.7).
akitaSummer
force-pushed
the
fix/remove_service_worker_egg_module
branch
from
bc4b8f0 to
bef6989
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
standalone/service-worker/package.json (1)
1-78:⚠️ Potential issue | 🔴 CriticalAdd
eggModulefield to package.json — critical for module loading.The
package.jsonis missing the requiredeggModulefield. WhenServiceWorkerAppregisters the framework dependency (baseDir:standalone/service-worker),ModuleLoadUnit.createModule()will attempt to load it as a module and assert thatpkg.eggModuleexists (core/metadata/src/impl/ModuleLoadUnit.ts:302). Without it, the loader will fail with "module config not found in package".
LoadUnitInnerClassHook([ServiceWorkerRunner])does not replace module registration — it only registers inner classes within an already-loaded module. TheContextProtoLoadUnitHook('serviceWorker')depends on a LoadUnit with name 'serviceWorker' existing, which cannot be created withouteggModule.Add
"eggModule": { "name": "serviceWorker" }topackage.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@standalone/service-worker/package.json` around lines 1 - 78, The package.json is missing the required eggModule entry which causes ModuleLoadUnit.createModule to assert (see ModuleLoadUnit.createModule) when ServiceWorkerApp registers the framework dependency; add an "eggModule" object to package.json with the module name "serviceWorker" (e.g. "eggModule": { "name": "serviceWorker" }) so the LoadUnit for 'serviceWorker' can be created and hooks like LoadUnitInnerClassHook([ServiceWorkerRunner]) and ContextProtoLoadUnitHook('serviceWorker') can register correctly.
🧹 Nitpick comments (2)
standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts (2)
14-19: Consider parallelizingcreateProtocalls.The inner loop awaits each
createProtosequentially. Since each call is independent perclazz,Promise.allwould let them run concurrently. Minor, and only matters ifinnerClassesgrows beyond[ServiceWorkerRunner].♻️ Proposed refactor
- for (const clazz of this.innerClasses) { - const protos = await EggPrototypeCreatorFactory.createProto(clazz, loadUnit); - for (const proto of protos) { - EggPrototypeFactory.instance.registerPrototype(proto, loadUnit); - } - } + const allProtos = await Promise.all( + this.innerClasses.map(clazz => EggPrototypeCreatorFactory.createProto(clazz, loadUnit)), + ); + for (const protos of allProtos) { + for (const proto of protos) { + EggPrototypeFactory.instance.registerPrototype(proto, loadUnit); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts` around lines 14 - 19, The loop in LoadUnitInnerClassHook is awaiting EggPrototypeCreatorFactory.createProto(clazz, loadUnit) sequentially over this.innerClasses; change it to launch all createProto calls concurrently (e.g., map this.innerClasses to an array of promises from EggPrototypeCreatorFactory.createProto), await Promise.all to get the protos results, then iterate the combined results and call EggPrototypeFactory.instance.registerPrototype(proto, loadUnit) for each proto; ensure you preserve the existing registerPrototype call and handle the nested arrays of protos returned per clazz when flattening the Promise.all result.
1-3: Align imports with codebase conventions.The factories
EggPrototypeCreatorFactoryandEggPrototypeFactoryshould be imported from@eggjs/tegg-metadata(not@eggjs/tegg/helper) for consistency withplugin/tegg/lib/AppLoadUnit.ts,standalone/standalone/src/StandaloneLoadUnit.ts, and other modules in the repository. Type-only imports likeLoadUnitandLoadUnitLifecycleContextshould useimport typeand come from@eggjs/tegg-types.Suggested fix
import { EggProtoImplClass, LifecycleHook } from '@eggjs/tegg'; import { EggPrototypeCreatorFactory, EggPrototypeFactory } from '@eggjs/tegg-metadata'; import type { LoadUnit, LoadUnitLifecycleContext } from '@eggjs/tegg-types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts` around lines 1 - 3, The imports are not aligned with project conventions: change the non-type imports for EggPrototypeCreatorFactory and EggPrototypeFactory to come from '@eggjs/tegg-metadata' (instead of '@eggjs/tegg/helper') and move LoadUnit and LoadUnitLifecycleContext to be type-only imports from '@eggjs/tegg-types' using `import type`; update the import statements at the top of LoadUnitInnerClassHook.ts where EggProtoImplClass, LifecycleHook, EggPrototypeCreatorFactory, EggPrototypeFactory, LoadUnit, and LoadUnitLifecycleContext are declared so the factories use '@eggjs/tegg-metadata' and the two lifecycle/type symbols use `import type` from '@eggjs/tegg-types'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@standalone/service-worker/package.json`:
- Line 4: The package.json "version" field in the standalone/service-worker
package is incorrectly set to "3.78.3" while its dependencies and sibling
packages use 3.78.7; update the "version" value to "3.78.7" (or a higher
compatible release) so it matches the monorepo and the dependency range (e.g.,
dependencies referencing ^3.78.7), ensuring the package can be published and
release consistency is preserved.
In `@standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts`:
- Around line 12-20: Replace the hard-coded string comparison in
LoadUnitInnerClassHook.postCreate with the exported constant: import
StandaloneLoadUnitType from the package and change the guard from loadUnit.type
!== 'StandaloneLoadUnitType' to loadUnit.type !== StandaloneLoadUnitType; also
update the standalone package public exports by adding export * from
'./src/StandaloneLoadUnit'; to standalone/standalone/index.ts so
StandaloneLoadUnitType is available from '@eggjs/tegg-standalone'.
In `@standalone/service-worker/src/ServiceWorkerApp.ts`:
- Line 58: Registering a new LoadUnitInnerClassHook([ServiceWorkerRunner]) via
LoadUnitLifecycleUtil.registerLifecycle without keeping the instance leaks the
hook; retain the created hook instance in the ServiceWorkerApp (e.g., a private
field like lifecycleHook) when you call LoadUnitLifecycleUtil.registerLifecycle,
and then call LoadUnitLifecycleUtil.deleteLifecycle(this.lifecycleHook) from the
destroy() method to unregister it; ensure the field is initialized where
registerLifecycle is currently called and cleaned up (set to null) after
deleteLifecycle to avoid duplicate firings on re-instantiation.
---
Outside diff comments:
In `@standalone/service-worker/package.json`:
- Around line 1-78: The package.json is missing the required eggModule entry
which causes ModuleLoadUnit.createModule to assert (see
ModuleLoadUnit.createModule) when ServiceWorkerApp registers the framework
dependency; add an "eggModule" object to package.json with the module name
"serviceWorker" (e.g. "eggModule": { "name": "serviceWorker" }) so the LoadUnit
for 'serviceWorker' can be created and hooks like
LoadUnitInnerClassHook([ServiceWorkerRunner]) and
ContextProtoLoadUnitHook('serviceWorker') can register correctly.
---
Nitpick comments:
In `@standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts`:
- Around line 14-19: The loop in LoadUnitInnerClassHook is awaiting
EggPrototypeCreatorFactory.createProto(clazz, loadUnit) sequentially over
this.innerClasses; change it to launch all createProto calls concurrently (e.g.,
map this.innerClasses to an array of promises from
EggPrototypeCreatorFactory.createProto), await Promise.all to get the protos
results, then iterate the combined results and call
EggPrototypeFactory.instance.registerPrototype(proto, loadUnit) for each proto;
ensure you preserve the existing registerPrototype call and handle the nested
arrays of protos returned per clazz when flattening the Promise.all result.
- Around line 1-3: The imports are not aligned with project conventions: change
the non-type imports for EggPrototypeCreatorFactory and EggPrototypeFactory to
come from '@eggjs/tegg-metadata' (instead of '@eggjs/tegg/helper') and move
LoadUnit and LoadUnitLifecycleContext to be type-only imports from
'@eggjs/tegg-types' using `import type`; update the import statements at the top
of LoadUnitInnerClassHook.ts where EggProtoImplClass, LifecycleHook,
EggPrototypeCreatorFactory, EggPrototypeFactory, LoadUnit, and
LoadUnitLifecycleContext are declared so the factories use
'@eggjs/tegg-metadata' and the two lifecycle/type symbols use `import type` from
'@eggjs/tegg-types'.
🪄 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: ecc4f753-9cf8-41c2-b4aa-0f0320e5de27
📒 Files selected for processing (4)
standalone/service-worker/index.tsstandalone/service-worker/package.jsonstandalone/service-worker/src/ServiceWorkerApp.tsstandalone/service-worker/src/hook/LoadUnitInnerClassHook.ts
💤 Files with no reviewable changes (1)
- standalone/service-worker/index.ts
akitaSummer
force-pushed
the
fix/remove_service_worker_egg_module
branch
from
7f32797 to
a1ec67c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
standalone/service-worker/src/StandaloneEggObjectFactory.ts (1)
7-10: Set the singleton name explicitly to match the injection token.
ServiceWorkerRunnerinjectsstandaloneEggObjectFactoryby name, but this prototype currently relies on derived naming. Make the DI contract explicit, mirroring the baseEggObjectFactorypattern.Proposed fix
`@SingletonProto`({ protoImplType: EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE, + name: 'standaloneEggObjectFactory', accessLevel: AccessLevel.PRIVATE, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@standalone/service-worker/src/StandaloneEggObjectFactory.ts` around lines 7 - 10, The SingletonProto decorator on class StandaloneEggObjectFactory currently relies on derived naming; explicitly set its singleton name to the injection token used by ServiceWorkerRunner by adding the name property to the decorator (match "standaloneEggObjectFactory"), following the same pattern as the base EggObjectFactory; update the `@SingletonProto`(...) invocation to include name: "standaloneEggObjectFactory" so DI resolves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@standalone/service-worker/src/ServiceWorkerRunner.ts`:
- Around line 2-12: The ServiceWorkerRunner class must explicitly implement the
MainRunner<unknown> interface: update the class declaration for
ServiceWorkerRunner (the class annotated with `@Runner`()) to implement
MainRunner<unknown> so its signature matches the standalone runner contract and
aligns with the existing async main(): Promise<unknown> method; ensure imports
include MainRunner if missing and keep the `@Runner`() decorator and existing main
method intact.
In `@standalone/service-worker/src/StandaloneEggObjectFactory.ts`:
- Around line 1-5: The import uses a deep /src path and a non-public symbol
which will break in published builds; add a public export for
EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE in the package's root export (e.g.,
core/dynamic-inject-runtime/index.ts) and then update the import in
StandaloneEggObjectFactory.ts to import { EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE }
from '@eggjs/tegg-dynamic-inject-runtime' (instead of from
.../src/EggObjectFactoryPrototype), ensuring you reference the symbol name
EGG_OBJECT_FACTORY_PROTO_IMPLE_TYPE and keep the rest of the existing imports
(AccessLevel, SingletonProto, EggObjectFactory) unchanged.
---
Nitpick comments:
In `@standalone/service-worker/src/StandaloneEggObjectFactory.ts`:
- Around line 7-10: The SingletonProto decorator on class
StandaloneEggObjectFactory currently relies on derived naming; explicitly set
its singleton name to the injection token used by ServiceWorkerRunner by adding
the name property to the decorator (match "standaloneEggObjectFactory"),
following the same pattern as the base EggObjectFactory; update the
`@SingletonProto`(...) invocation to include name: "standaloneEggObjectFactory" so
DI resolves correctly.
🪄 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: 8438385a-9847-4334-b16b-2af9b3ddbe8f
📒 Files selected for processing (7)
core/standalone-decorator/src/event/EventHandler.tsstandalone/service-worker/index.tsstandalone/service-worker/package.jsonstandalone/service-worker/src/ServiceWorkerApp.tsstandalone/service-worker/src/ServiceWorkerRunner.tsstandalone/service-worker/src/StandaloneEggObjectFactory.tsstandalone/service-worker/src/hook/LoadUnitInnerClassHook.ts
💤 Files with no reviewable changes (1)
- standalone/service-worker/index.ts
✅ Files skipped from review due to trivial changes (2)
- core/standalone-decorator/src/event/EventHandler.ts
- standalone/service-worker/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- standalone/service-worker/src/hook/LoadUnitInnerClassHook.ts
- standalone/service-worker/src/ServiceWorkerApp.ts
akitaSummer
force-pushed
the
fix/remove_service_worker_egg_module
branch
from
4c1a9d3 to
190fe83
Compare
1 participant
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit