prepare typescript 4.5 #1407

PodaruDragos · 2021-12-16T09:48:31Z

Prepare for TS 4.5

replace "" with '' to make it future eslint compatible repace some of the any's update all packages

codecov-commenter · 2022-03-13T12:12:29Z

Codecov Report

Merging #1407 (ef9b8e7) into master (1df31d6) will decrease coverage by 0.29%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master    #1407      +/-   ##
==========================================
- Coverage   99.85%   99.55%   -0.30%     
==========================================
  Files          53       53              
  Lines        1367     1363       -4     
  Branches      192      196       +4     
==========================================
- Hits         1365     1357       -8     
- Partials        2        6       +4

Impacted Files	Coverage Δ
src/annotation/lazy_service_identifier.ts	`100.00% <ø> (ø)`
src/constants/literal_types.ts	`100.00% <ø> (ø)`
src/container/container_snapshot.ts	`100.00% <ø> (ø)`
src/interfaces/interfaces.ts	`100.00% <ø> (ø)`
src/planning/plan.ts	`100.00% <ø> (ø)`
src/syntax/binding_to_syntax.ts	`100.00% <ø> (ø)`
src/utils/clonable.ts	`100.00% <ø> (ø)`
src/resolution/instantiation.ts	`96.66% <77.77%> (-1.67%)`	⬇️
src/container/container.ts	`98.85% <92.50%> (-1.15%)`	⬇️
src/annotation/decorator_utils.ts	`100.00% <100.00%> (ø)`
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df31d6...ef9b8e7. Read the comment docs.

jakehamtexas

Thank you @PodaruDragos for your beautiful work in this PR 🙏

I have a few things that I would like to see changes about, and I am happy to discuss them more:

Please reconsider the usage of never when defining a function type of unknown arity. (this tends to introduce complications with using the type and interacting with other types that depend on ...unknown[]).
Please reconsider making a member public without addressing the underlying code smell (please see my comment for a writeup about this).

jakehamtexas · 2022-03-15T10:53:28Z

src/utils/exceptions.ts


-export function isStackOverflowExeption(error: Error) {
+export function isStackOverflowExeption(error: unknown | Error): error is RangeError {


Since unknown is a top type, unknown | Error is the equivalent of simply unknown to the compiler.

Lovely to see the usage of the is RangeError type predicate though 👍

Suggested change

export function isStackOverflowExeption(error: unknown | Error): error is RangeError {

export function isStackOverflowExeption(error: unknown): error is RangeError {

I have noticed this situation of using SomeType | unknown at least one other time, and I will try to find it, but I know I may miss it.

Would Error | unknown work better?

No, unfortunately it is the same no matter the order @dcavanagh

Any type union with unknown will simply be unknown without any of the other union members

jakehamtexas · 2022-03-15T11:18:18Z

src/syntax/binding_to_syntax.ts

@@ -6,7 +6,7 @@ import { BindingWhenOnSyntax } from "./binding_when_on_syntax";

 class BindingToSyntax<T> implements interfaces.BindingToSyntax<T> {

-  private _binding: interfaces.Binding<T>;
+  public _binding: interfaces.Binding<T>;


For this, I think we should keep this member private and use the same tricks that are in use elsewhere to make it accessible with an as unknown as { _binding: interfaces.Binding<T> }, but only for now :). Please see my following comment.

I think that this is uncovering a smell.

I have noticed in the past that this member is private, but a type assertion is used in order to allow it to be accessed from a public location. This is a pattern that can be observed with other members, not just this one (for instance, _bindingDictionary).

A few places I have noticed this are among test files:

test/container/container.test.ts
test/syntax/binding_when_on_syntax.test.ts

Another place is in src/planning/planner.ts.

The reality of this smell is that there is a need for two forms of the containing types (in this case, I am referring to BindingToSyntax since this is where we have concentrated this specific example):

A presentational version which maintains a correct API for the consumers of the package, and does not expose unnecessary members like _binding.

An internal version which allows a more exposed API for the purposes of testing and proper layer integration with planner and friends.

I think that a good way of accomplishing this which resolves the smell is to rename the class with a _ prefix, as in BindingToSyntax => _BindingToSyntax, etc. This means that the current definition of this class will become the internal form of the class. Then, any members which are meant to be public may be accomplished in the following manner:

// Please ignore the lack of generic type vars, etc., // as this is only meant to be an example class BindingToSyntax { constructor( private _internal = new _BindingToSyntax(...args) ) {} get somePublicMemberFromOriginalImplementation() { return this._internal.somePublicMemberFromOriginalImplementation; } }

etc.

Finally, with a presentational/internal pattern like this, we are free to fix the original problem for those classes which have the behavior of needing a private member exposed for testing, or for use in planner or similar. We can simply make the members that needed this special access despite being private members into public ones instead.

If you agree @PodaruDragos then I will spin the above comment off as an issue for us to work on at some time.

totally agree with this. Do you also agree that we should just move everything to actual #private fields since you know they are really private ?

I think it is worth taking a look at cleaning this up

@PodaruDragos I think we should definitely do #private type of vars in the future, for now let's not increase the scope of this PR

jakehamtexas · 2022-03-15T11:31:33Z

src/planning/reflection_utils.ts

  let serviceIdentifier = serviceIdentifiers[index];
-  const injectIdentifier = (metadata.inject || metadata.multiInject);
-  serviceIdentifier = (injectIdentifier) ? (injectIdentifier) : serviceIdentifier;
+  const injectIdentifier = metadata.inject || metadata.multiInject;
+  serviceIdentifier = (injectIdentifier ? injectIdentifier : serviceIdentifier) as interfaces.ServiceIdentifier<unknown> | undefined;


I know it was just simple type annotations being added here, but since injectIdentifier isn't used outside of the assignment on line 80, it would be great if someday we could rewrite this in the following manner:

let serviceIdentifier = metadata.inject ?? metadata.multiInject ?? serviceIdentifiers[index];

Ideally this would avoid needing an explicit cast to interfaces.ServiceIdentifier<unknown> | undefined, since (I hope) we have strictNullChecks enabled.

This would be better enabled by making the return type of formatTargetMetadata use interfaces.ServiceIdentifier<unknown> for the inject and multiInject properties.

jakehamtexas · 2022-03-15T11:33:58Z

src/interfaces/interfaces.ts

@@ -90,7 +92,7 @@ namespace interfaces {
    factory: FactoryTypeFunction | null
  };

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


I would highly recommend leaving this type as any. never is not really a good candidate for this use case.

In my mind, any is acceptable for defining function args with unknown arity.

jakehamtexas · 2022-03-15T11:38:24Z

src/interfaces/interfaces.ts

@@ -40,6 +40,8 @@ namespace interfaces {

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

+  export type Instance<T> = T & Record<string, () => void>;


Interesting choice here. I think there may be a better name for this type as it relates to the context it is used in.

I like the idea of encapsulating this type with type keyword.

Calling it Instance feels a little undercooked since the right hand side of the assignment does not really imply to me that this type is exlusively Instance-like. Does it seem possible that we could infer a better name from its usage?

Should we consider whether this should be exported publicly from the package? We tend to put all interfaces in this module, which makes them available to consumers of the package, but thinking of proper encapsulation, does a consumer of the package care about this type? Is there a better/closer place to put?

I'm personally ready to challenge the assumption that using interfaces.ts for all interfaces and types is the correct pattern, but it's certainly not in scope for this one issue :) Maybe we should keep it here for now and thinking about breaking apart interfaces in a future breaking changes release?

@jakehamtexas I agree about the interfaces. interfaces.ts should really only export the interfaces we want to see in the public api.

jakehamtexas · 2022-03-15T11:39:45Z

src/bindings/binding_count.ts

@@ -1,7 +1,5 @@
-const BindingCount = {
+export const BindingCount = {


I like this a lot better. Maybe in the future this is better as an enum?

dcavanagh

Thanks for all the work. Looking good and generating some discussion!

dcavanagh · 2022-03-14T12:44:28Z

.github/workflows/ci.yml

@@ -91,4 +91,4 @@ jobs:
        uses: codecov/codecov-action@f32b3a3741e1053eb607407145bc9619351dc93b # renovate: tag=v2
        with:
          directory: coverage/
-          fail_ci_if_error: true
+          fail_ci_if_error: true


missing new line

dcavanagh · 2022-03-14T12:47:29Z

src/bindings/binding_count.ts

-};
-
-export { BindingCount };
+};


missing new line

dcavanagh · 2022-03-16T13:20:19Z

package.json

@@ -1,48 +1,9 @@
 {
-  "name": "inversify",


Not sure if we need this reorganized.

dcavanagh · 2022-03-16T13:22:36Z

src/interfaces/interfaces.ts

@@ -40,6 +40,8 @@ namespace interfaces {

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

+  export type Instance<T> = T & Record<string, () => void>;


@jakehamtexas I agree about the interfaces. interfaces.ts should really only export the interfaces we want to see in the public api.

dcavanagh · 2022-03-16T13:24:51Z

src/syntax/binding_to_syntax.ts

@@ -6,7 +6,7 @@ import { BindingWhenOnSyntax } from "./binding_when_on_syntax";

 class BindingToSyntax<T> implements interfaces.BindingToSyntax<T> {

-  private _binding: interfaces.Binding<T>;
+  public _binding: interfaces.Binding<T>;


I think it is worth taking a look at cleaning this up

dcavanagh · 2022-03-16T13:47:04Z

test/planning/context.test.ts

@@ -40,7 +40,7 @@ describe("Context", () => {
    const plan = new Plan(context, ninjaRequest);
    context.addPlan(plan);

-    expect(context.plan.rootRequest.serviceIdentifier).eql("Ninja");
+    expect(context.plan.rootRequest.serviceIdentifier).eql('Ninja');
  });

 });


missing new line

dcavanagh · 2022-03-16T13:50:07Z

test/utils/binding_utils.test.ts

+  it('should thrown an exception non factory binding.type', () => {
+    const binding = new Binding('', 'Singleton');
+    binding.type = 'Instance';
+    expect(() => getFactoryDetails(binding)).to.throw('Unexpected factory type Instance');
  });
 })


missing new line

dcavanagh · 2022-03-16T13:50:18Z

test/utils/id.test.ts

    const id1 = id();
-    expect(id1).to.be.a("number");
+    expect(id1).to.be.a('number');
  });

 });


missing newline

dcavanagh · 2022-03-16T13:50:31Z

test/utils/serialization.test.ts


    const symbolWithoutDescription = Symbol();
-    expect(getSymbolDescription(symbolWithoutDescription)).to.equal("");
+    expect(getSymbolDescription(symbolWithoutDescription)).to.equal('');
  });

 });


missing new line

dcavanagh · 2022-03-16T13:51:41Z

tslint.json

@@ -20,7 +20,10 @@
    "max-file-line-count": false,
    "max-line-length": [
      true,
-      140


I think I would rather have the imports have line breaks after 3

Perhaps better under a different scope? (See previous formatting comment)

Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6) --- updated-dependencies: - dependency-name: minimist dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Podaru Dragos <[email protected]>

PodaruDragos added 3 commits December 16, 2021 11:47

prepare typescript 4.5

05813c5

fix tests

5c90fe3

replace "" with '' to make it future eslint compatible repace some of the any's update all packages

Merge remote-tracking branch 'upstream/master' into update-packages

3d9a2c0

PodaruDragos requested review from dcavanagh, jakehamtexas and leonardssh March 13, 2022 11:41

PodaruDragos added 3 commits March 13, 2022 13:43

posible fix

7c19932

umbump packages since karma or phantom won't work with es6 features

7305bb4

fix coverage report

d0b450d

replace more any's

e61a08e

jakehamtexas requested changes Mar 15, 2022

View reviewed changes

dcavanagh requested changes Mar 16, 2022

View reviewed changes

Merge branch 'master' into update-packages

0b189e4

jakehamtexas self-requested a review May 21, 2022 16:29

jakehamtexas force-pushed the update-packages branch from fadfa2b to 6cc2d05 Compare May 21, 2022 17:13

dependabot bot and others added 4 commits May 21, 2022 12:21

do not use never over any when defining function signature type aliases

327a0e7

get rid of useless type union

bbe9c46

replace private member _binding and add TODO

fb2a91e

jakehamtexas force-pushed the update-packages branch from 6cc2d05 to fb2a91e Compare May 21, 2022 17:23

Merge branch 'master' into update-packages

ef9b8e7

PodaruDragos merged commit 3975ab2 into master May 28, 2022

PodaruDragos deleted the update-packages branch May 28, 2022 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prepare typescript 4.5 #1407

prepare typescript 4.5 #1407

PodaruDragos commented Dec 16, 2021

codecov-commenter commented Mar 13, 2022 •

edited

Loading

jakehamtexas left a comment

jakehamtexas Mar 15, 2022

jakehamtexas Mar 15, 2022

dcavanagh Mar 16, 2022

jakehamtexas May 21, 2022

jakehamtexas Mar 15, 2022

jakehamtexas Mar 15, 2022 •

edited

Loading

jakehamtexas Mar 15, 2022

PodaruDragos Mar 15, 2022

dcavanagh Mar 16, 2022

jakehamtexas May 21, 2022

jakehamtexas Mar 15, 2022

jakehamtexas Mar 15, 2022

jakehamtexas Mar 15, 2022

dcavanagh Mar 16, 2022

jakehamtexas Mar 15, 2022

dcavanagh left a comment

dcavanagh Mar 14, 2022

dcavanagh Mar 14, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

dcavanagh Mar 16, 2022

jakehamtexas May 21, 2022


		export function isStackOverflowExeption(error: Error) {
		export function isStackOverflowExeption(error: unknown \| Error): error is RangeError {

		@@ -40,6 +40,8 @@ namespace interfaces {

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

		export type Instance<T> = T & Record<string, () => void>;

		@@ -1,7 +1,5 @@
		const BindingCount = {
		export const BindingCount = {

prepare typescript 4.5 #1407

prepare typescript 4.5 #1407

Conversation

PodaruDragos commented Dec 16, 2021

codecov-commenter commented Mar 13, 2022 • edited Loading

Codecov Report

jakehamtexas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakehamtexas Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcavanagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 13, 2022 •

edited

Loading

jakehamtexas Mar 15, 2022 •

edited

Loading