Update Inversify to latest #14435
Open
Update Inversify to latest #14435
Conversation
This file contains 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
sdirix
reviewed
Nov 12, 2024
msujew
force-pushed
the
msujew/prepare-inversify
branch
from
270eb10
to
0fd9f9d
Compare
sdirix
reviewed
Nov 12, 2024
There was a problem hiding this comment.
I was able to build and start the browser application and did not encounter any error just by clicking a bit around and trying some features.
I then checked with the regex @injectable(.|\n)*?constructor to find all places where an @unmanaged might be needed.
I found the following @injectable classes which either have constructor parameters which are neither injected nor are declared unmanaged or execute a super call to a parent class without such declarations for their parameters.
|
@sdirix Thanks for looking into this! I've added the decorations for the listed classes :) |
msujew
force-pushed
the
msujew/prepare-inversify
branch
from
d21a7ce
to
f21e842
Compare
sdirix
approved these changes
Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What it does
Closes #14431.
The latest minor version of Inversify (6.1.0) introduced some behavior changes with regards to injected base classes. Inversify validates that the constructor arguments are in some way satisfied (by asserting constructor function length). Since we sometimes use an inheritance pattern that breaks this assertion, this leads to runtime failures in newer versions of Inversify.
This change adds
@unmanaged()decorations to all constructors that aren't injected. Also adds a few more type constraints during compile time where Inversify got more strict in its typing.How to test
Assert that the CI is green, and the application builds and runs without any start error.
Follow-ups
I couldn't really find all places where we're using the above mentioned pattern, as I couldn't find a way to easily search for it.
Review checklist
Reminder for reviewers