-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update inversify to fix TypeScript 5.0 errors #12425
Conversation
I've noticed a few issues with this PR. I'll put it on hold for now. |
To explain the issues in more detail: We cannot inject promises into services with inversify 6. This isn't so bad, as we can easily work around this. However, services are now not allowed to contain promise fields at construction time. The following class can no longer be injected due to inversify incorrectly identifiying it as an "async class": class InjectableService {
constructor() {
this.deferred = new Deferred(); // <-- This contains a promise, which makes this class only injectable in an async context.
}
} See also inversify/InversifyJS#1482 for more context. |
Your code snippet wouldn't actually cause any issue with Note that I encountered the same issue and opened a PR on Inversify: inversify/InversifyJS#1461 The reason I closed it is because it really feels like a DI anti-pattern to inject promises. When practising dependency injection you want to inject abstractions. Injecting a promise to an abstraction leads to problems such as handling delayed initialization of classes depending on promises that will eventually resolve. So Inversify went the route of "injecting a promise means we need to delay the initialization of your component until it resolves to the actual dependency that actually should be injected". The quick fix for that specific issue would be to convert all bindings to promises into Inversify providers: // before:
bind(OurPromiseApi).toConstantValue(thePromise);
// after:
bind(OurPromiseApi).toProvider(ctx => async () => thePromise); Then we'll need to replace injection sites like |
@paul-marechal Yeah, you're right. I misinterpreted the error message that I got. The issue was with the async |
Ah true, the async post constructors are also a problem when using Related: #10788 |
@paul-marechal This is ready for a review now. Note that I had to change something in the rpc-proxy mechanism, since Inversify thought that every proxy that we use as a dependency is in fact a I'll add all the breaking changes after this gets an initial review. There are quite a few breaking changes in this PR, and I want a review on the general approach before documenting them all. |
packages/core/src/browser/keyboard/browser-keyboard-layout-provider.ts
Outdated
Show resolved
Hide resolved
All that's missing now is an entry to the migration guide? Since we share our version of Inversify with downstream, their apps will break if they have async post constructors too. |
@paul-marechal I've updated the migration guide and documented the breaking changes (at least everything that wasn't just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM, will merge right after the upcoming release this week.
Just caught async dep issues on Electron and managed to fix them. Please have one last look and merge if you don't find any other regressions/issues. |
What it does
With TypeScript 5.0, parameter checking for decorators has been made a bit stricter, which seems to clash with the type definitions of
inversify@^5.0.0
.inversify@^6.0.0
seems to resolve this. This is also related to eclipse-theia/generator-theia-extension#164.How to test
The CI should be green.
Review checklist
Reminder for reviewers