Update typescript compiler options #1311

notaphplover · 2021-04-18T13:05:04Z

Description

This PR aims to update Typescript version and add strict compiler options

Related Issue

Motivation and Context

We should try to use Typescript strict options in our project. This would help us to detect some typing issues such as #1222, which would have been detected.

How Has This Been Tested?

All tests passes successfully

Types of changes

Updated docs / Refactor code / Added a tests case (non-breaking change)
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:

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

…o update-typescript-compiler-options

src/planning/planner.ts

notaphplover · 2021-04-19T13:05:46Z

package.json

@@ -71,7 +71,7 @@
    "ts-node": "9.1.1",
    "tsify": "^5.0.2",
    "tslint": "6.1.3",
-    "typescript": "4.1.3",
+    "typescript": "^4.2.4",


Typescript creates false minor versions, we should ensure we are using the right version

Suggested change

"typescript": "^4.2.4",

"typescript": "4.2.4",

fix all failing test fix all failing build code

…-options

PodaruDragos · 2021-06-15T07:35:30Z

@tonyhallett @notaphplover I really want to get this merged eventually. Things at this moment should be correct for building but tests are failing and I really don't know what's happening there. Maybe some of you can have a second opinion on this.

tonyhallett · 2021-06-15T08:39:28Z

@PodaruDragos I am busy finishing a project at the moment. Might get the chance to take a look tomorrow or Thursday

PodaruDragos · 2021-06-15T13:46:39Z

@PodaruDragos I am busy finishing a project at the moment. Might get the chance to take a look tomorrow or Thursday

no worries, take your time

…-options

PodaruDragos · 2021-08-25T09:14:20Z

@tonyhallett @notaphplover I've kept this branch up to date. There are 2 issues when running test and 1 issue when running test:browser and build.

Can you also take a look at what's going on there maybe you have a clearer picture on how to solve those issues.
Then I think we can merge this safely and focus on the binding-abstractions with stricter typescript.

tonyhallett · 2021-08-25T12:04:30Z

Looking at it now

tonyhallett · 2021-08-25T12:53:01Z

Rectified with change to

    public toAutoNamedFactory<T2>(serviceIdentifier: interfaces.ServiceIdentifier<T2>): BindingWhenOnSyntax<T> {
        this._binding.type = BindingTypeEnum.Factory;
        this._binding.factory = (context) => {
            return (named: unknown) => context.container.getNamed<T2>(serviceIdentifier, named as string);
        };
        return new BindingWhenOnSyntax<T>(this._binding);
    }

tonyhallett · 2021-08-25T13:01:30Z

I would also say that the pull by @leonardssh has introduced an issue in the typings.

export type Factory<T, U extends unknown[] = unknown[]> = (...args: U) => (((...args: U) => T) | T);

When

 (...args: U) => (...args: U) => T

Why should the second arguments be the same as the first ?

This error can be seen in

InversifyJS/test/inversify.test.ts

Line 900 in 731aa75

    
           container.bind<interfaces.Factory<Engine>>("Factory<Engine>").toFactory<Engine>((context: interfaces.Context) =>

        container.bind<interfaces.Factory<Engine>>("Factory<Engine>").toFactory<Engine>((context: interfaces.Context) =>
            (theNamed: string) => (displacement: number) => {
                const theEngine = context.container.getNamed<Engine>("Engine", theNamed);
                theEngine.displacement = displacement;
                return theEngine;
            });

Also note that because of unknown you have to provide the second generic to 'toFactory' ( or have unknown parameters )

tonyhallett · 2021-08-25T14:02:48Z

After clearing the issue identified by ci build I then noticed in my error list that there were 50ish other errors with many that are not pertaining to 'update typescript compiler options' but are a result of the merge and the fix leaving the code in this present state.

Please resolve resolver.test.ts and get Factory/FactoryCreator sorted.
Note that Provider/ProviderCreator have not changed.

InversifyJS/src/interfaces/interfaces.ts

Line 85 in 731aa75

    
           export type Provider<T> = (...args: any[]) => (((...args: any[]) => Promise<T>) | Promise<T>);

Then I will help look at the decorator issues and any strange ci browser test issue that may present itself.

PodaruDragos · 2021-08-25T20:06:17Z

@tonyhallett checked the branch right now, i don't have any issues but the one already mentioned

PodaruDragos · 2021-08-25T20:30:34Z

I also formatted the code to have 2 spaces all over the place.. The 4 and 2 spaces randomly was really freaking me out.
This was also agreed with Dan when we eventually do finish this and move to eslint that we should use 2 spaces for formatting.

@tonyhallett hope that didn't cause you any discomfort.

tonyhallett · 2021-08-25T20:33:21Z

checked the branch right now, i don't have any issues but the one already mentioned

I will check to see what stupid mistake I made - apologies in advance

tonyhallett · 2021-09-03T10:00:30Z

@PodaruDragos
Please can you tell me what comment of mine you deleted

PodaruDragos · 2021-09-03T10:09:51Z

@PodaruDragos
Please can you tell me what comment of mine you deleted

yeah sorry , the one with cast to number that I modified to be as string.

tonyhallett · 2021-09-03T10:13:05Z

yeah sorry , the one with cast to number that I modified to be as string.

Ok 😉

tonyhallett · 2021-09-03T10:14:56Z

wiki/provider_injection.md

@@ -58,7 +58,7 @@ The signature of a provider look as follows:

 ```ts
 interface Provider<T> extends NewableFunction {
-    (...args: any[]): (((...args: any[]) => Promise<T>) | Promise<T>);
+    (...args: unknown[]): (((...args: unknown[]) => Promise<T>) | Promise<T>);


Should we replace the interface with the type that we are exporting ? Similarly with ProviderCreator.

tonyhallett · 2021-09-03T12:29:22Z

src/interfaces/interfaces.ts

-  export type Factory<T, U extends unknown[] = unknown[], V extends unknown[] = unknown[]> = (...args: U) => (((...args: V) => T) | T);
+  export type SimpleFactory<T, U extends unknown[] = unknown[]> =  (...args: U) => T;
+
+  export type MultiFactory<T, U extends unknown[] = unknown[], V extends unknown[] = unknown[]> = (...args: U) => SimpleFactory<T,V>


@PodaruDragos
Helper types added. We need a better name for MultiFactory

We need some of the markdown files to be generated from actual tests so that required updates to markdown automatically occur when we change types. Will also prevent incorrect typing that I encountered when adding new functionality.

what about CompoundFactory ?
I also think MultiFactory is fine

PodaruDragos · 2021-09-03T13:03:33Z

Please revert / adjust accordingly
getDependencies
getBaseClassDependencyCount
listRegisteredBindingsForServiceIdentifier

this should also be done

tonyhallett · 2021-09-03T13:06:56Z

Please revert / adjust accordingly
getDependencies
getBaseClassDependencyCount
listRegisteredBindingsForServiceIdentifier

this should also be done

You did two of them, I did the other.

PodaruDragos · 2021-09-03T13:07:57Z

cntnr should be cast to object with _bindingDictionary

why are we not making this public ?

PodaruDragos · 2021-09-03T13:08:23Z

You did two of them, I did the other.

oh sorry, I must have slipped that

tonyhallett · 2021-09-03T13:10:24Z

why are we not making this public ?

We want to control access through fluent syntax

PodaruDragos · 2021-09-03T13:11:32Z

also we have a lot of tests where we do this

// cast to any to be able to access private props. maybe we could consider making those public aswell, since we are not really using private methods. or change everything to be private methods ex: #example = () => void

tonyhallett · 2021-09-03T15:08:27Z

also we have a lot of tests where we do this

// cast to any to be able to access private props. maybe we could consider making those public aswell, since we are not really using private methods. or change everything to be private methods ex: #example = () => void

The binding tests are poor.
There are multiple tests of this form

  it("Should set its own properties correctly", () => {

    interface Ninja { }
    const ninjaIdentifier = "Ninja";

    const binding = new Binding<Ninja>(ninjaIdentifier, BindingScopeEnum.Transient);
    
    const bindingInSyntax = new BindingInSyntax<Ninja>(binding);

    // cast to any to be able to access private props
    const _bindingInSyntax: any = bindingInSyntax;

    expect(_bindingInSyntax._binding.serviceIdentifier).eql(ninjaIdentifier);

  });

Why are we checking the serviceIdentifier ! Should be === on the binding.
Also for BindingWhenOnSyntax and BindingInWhenOnSyntax the _binding field is not used !
The test for BindingInSyntax is not required as we are testing that the scope gets changed !

binding_when_on_syntax.test.ts and binding_in_when_on_syntax.test.ts

Both of these just pass on, yet they do not test that they call with the correct parameter or return correctly !

Given that these types are never exposed there is no reason why we cannot make them public readonly

Also given that these are all exposed with interfaces is there any reason why BindingToSyntax isn't the only binding syntax class ?

PodaruDragos · 2021-09-03T15:39:16Z

Also given that these are all exposed with interfaces is there any reason why BindingToSyntax isn't the only binding syntax class ?

I don't really understand what you are asking here.

I have one more thing I want to propose. What if we do this export type Newable<T> = new (...args: never) => T; this way we drop the any and it behaves exactly the same ( we still don't have any typesafety for the ...args ) and then where we need to or overlap with other ...args we just cast them from unknown to never. I think just toConstructor does that. What are your thoughts on this ?

Also do you think there is a way to properly cast target in injectable , decorate , _decorate just so it's not any anymore.

Also regarding @multipleMetadataDecorator

    @inject<Thing>("Thing")
    @inject("Thing") thing: Thing

Is this not more or less the same thing ?

tonyhallett · 2021-09-03T17:03:19Z

Also given that these are all exposed with interfaces is there any reason why BindingToSyntax isn't the only binding syntax class ?

I don't really understand what you are asking here.

I am saying why have separate binding syntax classes. Why not put all functionality in a single class that inherits all interfaces - BindingToSyntax given that it is the entry. The return values are interfaces and ensure correct call order. The single class just returns itself for the next call.

Also regarding @multipleMetadataDecorator

    @inject<Thing>("Thing")
    @inject("Thing") thing: Thing

I assume that you mean replace injecting in setter with injecting the field.
I am injecting in a setter for a reason. So they are not the same thing at all.

export type Newable = new (...args: never) => T;

Why are you doing this ?
Have you tried it throughout the code ?

toConstructor should really be
toConstructor(constructor: T)
but we do toConstructor<T2>(constructor: Newable<T2>): so that it can be newed.

I think that we should do this

public toConstructor<T2>(constructor: interfaces.Newable<T2>)): interfaces.BindingWhenOnSyntax<T> {
    this._binding.type = BindingTypeEnum.Constructor;
    this._binding.implementationType = constructor as unknown as T;

Going to have to come back to you

Also do you think there is a way to properly cast target in injectable , decorate , _decorate just so it's not any anymore.

PodaruDragos · 2021-09-03T17:09:11Z

I assume that you mean replace injecting in setter with injecting the field.
I am injecting in a setter for a reason. So they are not the same thing at all.

my apologies, didn't actually check the code ( I was curios just about the typings )

I am saying why have separate binding syntax classes. Why not put all functionality in a single class that inherits all interfaces - BindingToSyntax given that it is the entry. The return values are interfaces and ensure correct call order. The single class just returns itself for the next call.

That would be better in every way, yes.

Why are you doing this ?
Have you tried it throughout the code ?

Yes seems to work, will have to double check tough.

Going to have to come back to you

looking forward

remove more any's remove casts to any make metadata generic

PodaruDragos · 2021-09-08T12:08:51Z

@tonyhallett I have pushed another commit ( I honestly don't know how to get rid of the remaining any's.
Do you have time to maybe look at that ?

I also resolved your feedback.
Let me know what you think.

tonyhallett · 2021-09-08T13:40:04Z

@PodaruDragos

Going to be busy for a few days. Will have a look soon.

jakehamtexas · 2021-10-07T14:23:30Z

@PodaruDragos In case it adds any context, this looks to me like where the error in the test case may be originating from:

It looks like _createSubRequests is failing as expected given the nature of the test case, but that context.plan is undefined for some reason.

Looking at the body of _createSubRequests, it appears that what could be throwing is the invocation of _getActiveBindings.

I'm inferring this because we use context.addPlan after that invocation, which would prevent context.plan from being undefined in the parent scope. I'm a little in the weeds trying to understanding the _getActiveBindings method, so I'm not sure how much more investigation I can manage.

An interesting final piece of context, it seems about five months ago this change was made in the commit history which makes accessing context.plan.rootRequest more unsafe:

Here is the associated PR: #1132

I am willing to bet the original code was meant to cover up an intermittent regression in accessing this property, and it was deemed unnecessary in the new PR and was thus removed. Maybe it was not time to move it yet and the root problem is still unsolved, or has returned?

Here is a comment in the PR which discusses the change: https://github.com/inversify/InversifyJS/pull/1132/files#r623122372

cc: @tonyhallett

tonyhallett · 2021-10-07T18:39:15Z

I will look to see if the removal of the code was a bad idea. It seems like it was.

notaphplover added 5 commits April 15, 2021 22:05

refactor: update typescript compiler options to be strict

33fd638

Merge branch 'master' of https://github.com/inversify/InversifyJS int…

9003f1e

…o update-typescript-compiler-options

test: add more specific type to match onActivation callback

f221dfb

test: update inversify.tests to compile

0a62966

Merge branch 'master' of https://github.com/inversify/InversifyJS int…

811e1be

…o update-typescript-compiler-options

notaphplover assigned PodaruDragos and notaphplover Apr 18, 2021

notaphplover mentioned this pull request Apr 18, 2021

Annotating dependencies with inversify.inject() and inversify.decorate() produces a TypeScript type error #1222

Closed

PodaruDragos reviewed Apr 18, 2021

View reviewed changes

src/planning/planner.ts Outdated Show resolved Hide resolved

notaphplover commented Apr 19, 2021

View reviewed changes

Dragos Podaru and others added 4 commits May 1, 2021 16:48

merge upstream master

29e5261

fix all failing test fix all failing build code

test

f3c7db0

remove test

829ad1a

Merge remote-tracking branch 'origin' into update-typescript-compiler…

27dd042

…-options

PodaruDragos mentioned this pull request Aug 23, 2021

V6 binding abstractions #1336

Draft

10 tasks

PodaruDragos added 3 commits August 23, 2021 23:20

Merge branch 'master' into update-typescript-compiler-options

a9c2dba

Merge remote-tracking branch 'origin' into update-typescript-compiler…

56c9602

…-options

fix merging issues

22137a6

add 2 spaces to all files

3aa77de

tonyhallett reviewed Sep 3, 2021

View reviewed changes

tonyhallett added 2 commits September 3, 2021 13:22

update md for FactoryCreator and introduce helper types

5908432

trailing whitespace

ee0a249

tonyhallett reviewed Sep 3, 2021

View reviewed changes

This was referenced Sep 3, 2021

Improve "toFactory" type safety #1370

Merged

Constrain node typings to node-only code and dom typings to browser-only code #1375

Open

tony's feedback

56aa549

remove more any's remove casts to any make metadata generic

PodaruDragos and others added 2 commits September 10, 2021 12:38

remove more any's

e37f845

Merge branch 'master' into update-typescript-compiler-options

9a32133

Merge branch 'master' into update-typescript-compiler-options

552b8cf

dcavanagh marked this pull request as ready for review October 7, 2021 23:32

dcavanagh merged commit 772ea8e into master Oct 7, 2021

dcavanagh deleted the update-typescript-compiler-options branch October 7, 2021 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update typescript compiler options #1311

Update typescript compiler options #1311

notaphplover commented Apr 18, 2021 •

edited

Loading

notaphplover Apr 19, 2021

PodaruDragos Apr 19, 2021

PodaruDragos commented Jun 15, 2021

tonyhallett commented Jun 15, 2021

PodaruDragos commented Jun 15, 2021

PodaruDragos commented Aug 25, 2021 •

edited

Loading

tonyhallett commented Aug 25, 2021

tonyhallett commented Aug 25, 2021

tonyhallett commented Aug 25, 2021 •

edited

Loading

tonyhallett commented Aug 25, 2021

PodaruDragos commented Aug 25, 2021

PodaruDragos commented Aug 25, 2021

tonyhallett commented Aug 25, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

tonyhallett Sep 3, 2021

tonyhallett Sep 3, 2021

tonyhallett Sep 3, 2021

PodaruDragos Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021 •

edited

Loading

PodaruDragos commented Sep 3, 2021

PodaruDragos commented Sep 8, 2021

tonyhallett commented Sep 8, 2021

jakehamtexas commented Oct 7, 2021 •

edited

Loading

tonyhallett commented Oct 7, 2021

Update typescript compiler options #1311

Update typescript compiler options #1311

Conversation

notaphplover commented Apr 18, 2021 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

notaphplover Apr 19, 2021

Choose a reason for hiding this comment

PodaruDragos Apr 19, 2021

Choose a reason for hiding this comment

PodaruDragos commented Jun 15, 2021

tonyhallett commented Jun 15, 2021

PodaruDragos commented Jun 15, 2021

PodaruDragos commented Aug 25, 2021 • edited Loading

tonyhallett commented Aug 25, 2021

tonyhallett commented Aug 25, 2021

tonyhallett commented Aug 25, 2021 • edited Loading

tonyhallett commented Aug 25, 2021

PodaruDragos commented Aug 25, 2021

PodaruDragos commented Aug 25, 2021

tonyhallett commented Aug 25, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

tonyhallett Sep 3, 2021

Choose a reason for hiding this comment

tonyhallett Sep 3, 2021

Choose a reason for hiding this comment

tonyhallett Sep 3, 2021

Choose a reason for hiding this comment

PodaruDragos Sep 3, 2021

Choose a reason for hiding this comment

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021

PodaruDragos commented Sep 3, 2021

tonyhallett commented Sep 3, 2021 • edited Loading

PodaruDragos commented Sep 3, 2021

PodaruDragos commented Sep 8, 2021

tonyhallett commented Sep 8, 2021

jakehamtexas commented Oct 7, 2021 • edited Loading

tonyhallett commented Oct 7, 2021

notaphplover commented Apr 18, 2021 •

edited

Loading

PodaruDragos commented Aug 25, 2021 •

edited

Loading

tonyhallett commented Aug 25, 2021 •

edited

Loading

tonyhallett commented Sep 3, 2021 •

edited

Loading

jakehamtexas commented Oct 7, 2021 •

edited

Loading