fix(setupWorker): resolve the TS4094 error #1477
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7be85e0:
|
src/setupWorker/setupWorker.ts
Outdated
| @@ -19,14 +19,15 @@ import { createFallbackStop } from './stop/createFallbackStop' | |||
| import { devUtils } from '../utils/internal/devUtils' | |||
| import { SetupApi } from '../SetupApi' | |||
| import { mergeRight } from '../utils/internal/mergeRight' | |||
| import { SetupWorkerApi as ISetupWorkerApi } from './glossary' | |||
There was a problem hiding this comment.
I think we need to export the SetupWorkerApi interface from index.ts just as we do SetupServerApi from node/index.ts. The fact that it wasn't exported before is a mistake.
There was a problem hiding this comment.
In fact it was already the interface that was exported, and not the class !
src/setupWorker/setupWorker.ts
Outdated
|
|
||
| interface Listener { | ||
| target: EventTarget | ||
| eventType: string | ||
| callback: EventListener | ||
| } | ||
|
|
||
| export class SetupWorkerApi extends SetupApi<WorkerLifecycleEventsMap> { | ||
| class SetupWorkerApi extends SetupApi<WorkerLifecycleEventsMap> { |
There was a problem hiding this comment.
Let's also force this class to implement the SetupWorker interface. Without this enforcement, the only area to catch the incongruence is in the setupWorker return type, which is rather late.
import { SetupWorker } from './glossary'
export class SetupWorkerApi extends SetupApi implements SetupWorker {
...
}
export function setupWorker(): SetupWorker { ... }- Rename
SetupWorkerApiinglossarytoSetupWorker- Do the same for
SetupServerApi
- Do the same for
- Export both
SetupWorkerinterface andSetupWorkerApiclass - Set
SetupWorkeras the return type forsetupWorker()to fix the type issue.
Would this approach work? 🤔
There was a problem hiding this comment.
I agree with the implements part, I hesitated to do that.
But as I said above the SetupWorkerApi interface is already exposed, so we should not change its name to avoid a breaking change.
The class was only exposed through setupWorker return type. And it cannot be used due to those TS declaration file limitations.
Tell me what you think and I'll update the PR
There was a problem hiding this comment.
Makes sense. Let's split this change in two then:
- Import
SetupWorkerApitype, alias it toSetupWorker, adjust the return type ofsetupWorkerfunction. Release as a fix. - Open a follow-up that exports the
SetupWorkerApi(class) and renames theSetupWorkerApi(interface) inglossary.tstoSetupWorker. Do the same forSetupServer*. Release a breaking change.
What do you think?
There was a problem hiding this comment.
Seems good. I had to check, but in fact exporting the class instead of the interface removes the TS error (I was expecting to still have it)
There was a problem hiding this comment.
I updated this one, I try to open the second one on top of this one
gduliscouet-ubitransport
force-pushed
the
fix/setup-worker-return-type
branch
from
4562c01 to
7be85e0
Compare
There was a problem hiding this comment.
Great work! Thank you for the discussion and the fix for this issue, @gduliscouet-ubitransport 👏 Welcome to contributors.
kettanaito
changed the title
Released: v0.49.1 🎉This has been released in v0.49.1! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Type the return of
setupWorkerwith the interface instead of the class.I also don't export the class
SetupWorkerApianymore because it was not used in other files in the project, and not exposd in theindex.ts(only the interface is exposed)I tested in local that it fixes #1454