Implement the proposed fix from issue #1290

[petetronic]

Implements the proposed fix from issue #1290 to satisfy tighter parameter type checking in latest versions of TypeScript.

Description

Without this change, if strict type checking is enabled in the latest versions of TypeScript, usages of a @unmanaged() decorator would cause the following error:

Argument of type 'undefined' is not assignable to parameter of type 'string'.

This can be seen using the most recently nightly builds of TypeScript 5.0.0 from late January 2023 onwards.

Related Issue

1290

Motivation and Context

Avoid compilation error in latest versions of TypeScript.

How Has This Been Tested?

Types of changes

• [ ] Updated docs / Refactor code / Added a tests case (non-breaking change)
• [X] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [X] My code follows the code style of this project.
• [NA] My change requires a change to the documentation.
• [NA] I have updated the documentation accordingly.
• [X] I have read the CONTRIBUTING document.
• [ ] I have added tests to cover my changes.
• [X] All new and existing tests passed.
• [ ] I have updated the changelog.

[linonetwo]

Use legacy decorator before this is merged

  "compilerOptions": {
    /* Wait until https://github.com/inversify/InversifyJS/pull/1499 fixed */
    "experimentalDecorators": true /* Enables experimental support for ES7 decorators. */,
    "emitDecoratorMetadata": true /* Enables experimental support for emitting type metadata for decorators. */
  },

[janeisenmenger]

Use legacy decorator before this is merged

This does not work for me. In the case of unmanaged(), we're still in a broken state with the legacy decorators - until this change in here is made, of course.

[mscharley]

@notaphplover is there anything still holding this up from being merged? It would be great to get this out so that people can start using typescript 5.x with inversify.

[linjer]

I'm also waiting for this patch for TS 5.x compatibility. Anything we can do to help get this merged? @PodaruDragos @notaphplover @dcavanagh

[Kalin8900]

This issue is taking time, and I don't see a reason why it's not yet merged. I'm still waiting for it to be resolved but in the meantime, if you need to use the decorator you can simply create a wrapper for it.

/**
 * @deprecated - It's just a wrapper for the inversify bug. There is a type mismatch that should not exist and the fix is not merged yet.
 * {@link https://github.com/inversify/InversifyJS/pull/1499}
 */
function unmanagedWrapper() {
  return (target: object, propertyKey: string | symbol | undefined, index: number) => {
    return unmanaged()(target, propertyKey as any, index);
  };
}

[khteh]

When will this be merged?

[jhechtf]

I too would like to know when this will be merged? Seems like it is waiting on some tests. Is it possible for someone who is a member of the inversify team to run them to verify so we can get this released?

[jsagethuboo]

any update on this?

[PodaruDragos]

So it seems the CI is not working anymore, I can't do anything about that. @dcavanagh @remojansen @Jameskmonger ?

[jamesjtb]

Tiny bump. This is holding us back from upgrading to TS5 as well.

[PodaruDragos]

Tiny bump. This is holding us back from upgrading to TS5 as well.

you will have to tag someone who has the rights to fix the CI..

[jamesjtb]

I'll just go off of how you tagged in your comment. @dcavanagh @remojansen @Jameskmonger ?

[dcavanagh]

@jamesjtb I think i fixed the CI. you just need to trigger a change in the PR. Edit your commit and force push it

[jamesjtb]

@dcavanagh Thanks! Unfortunately this isn't my PR/commit. @petetronic ?

[linjer]

@dcavanagh Not sure if the CI is still broken, but it looks like it's been stuck in the expected state for a few days. Is anything else needed, e.g., rebasing off latest main?

[codecov-commenter]

Codecov Report

Merging #1499 (36f9318) into master (01e5798) will not change coverage. Report is 1 commits behind head on master. The diff coverage is 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #1499   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          53       53           
  Lines        1566     1566           
  Branches      205      205           
=======================================
  Hits         1560     1560           
  Partials        6        6           

Files Changed	Coverage Δ
src/annotation/unmanaged.ts	100.00% <100.00%> (ø)
src/constants/error_msgs.ts	100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more