Skip to content
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 latest #14435

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Update Inversify to latest #14435

merged 4 commits into from
Nov 18, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Nov 12, 2024

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

@msujew msujew added the dependencies pull requests that update a dependency file label Nov 12, 2024
@msujew msujew requested a review from sdirix November 12, 2024 10:31
@msujew msujew force-pushed the msujew/prepare-inversify branch from 270eb10 to 0fd9f9d Compare November 12, 2024 13:10
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sdirix linked an issue Nov 12, 2024 that may be closed by this pull request
@msujew
Copy link
Member Author

msujew commented Nov 12, 2024

@sdirix Thanks for looking into this! I've added the decorations for the listed classes :)

@msujew msujew force-pushed the msujew/prepare-inversify branch from d21a7ce to f21e842 Compare November 13, 2024 11:06
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! Thanks for the work!

@tsmaeder
Copy link
Contributor

What's keeping this PR from being merged?

@msujew msujew merged commit 6103709 into master Nov 18, 2024
11 checks passed
@msujew msujew deleted the msujew/prepare-inversify branch November 18, 2024 08:41
@github-actions github-actions bot added this to the 1.56.0 milestone Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

samples generated by yo can not be used nor built
3 participants