Turn on noImplicitAny and strictNullChecks #153

shamsartem · 2022-05-10T09:18:54Z

In this pull request I turned on noImplicitAny and strictNullChecks. I think these are important rules that could prevent us from some bugs in the future. I also did some accidental refactoring here and there.

Nothing set in stone - everything could be discussed and reverted if needed

There is stuff in the pull request that could be refactored further, but I decided to close this pull request as soon as typescript is satisfied. If we decide additional changes are required - I will do them in a separate pull request

coder11

Awesome work!

coder11 · 2022-05-11T09:34:40Z

src/__test__/integration/sigService.spec.ts


        expect(result.success).toBe(true);
-        const isSigCorrect = await customSig.verify(result.signature, data);
+        const isSigCorrect = await customSig.verify(result.signature as number[], data);


Why not just result.signature! ?

this is needed to satisfy typescript. Because it's a test and we are sure it's not null - it's ok to just use "as" keyword. In other cases compiler will complain now so we have something to think about and maybe check explicitly that it's not null if needed

I mean use bang symbol

const isSigCorrect = await customSig.verify(result.signature!, data);

coder11 · 2022-05-11T09:35:07Z

src/__test__/integration/sigService.spec.ts


        const callAsSigRes = await callSig(peer, 'sig');
-        const callAsPeerIdRes = await callSig(peer, peer.getStatus().peerId);
+        const callAsPeerIdRes = await callSig(peer, peer.getStatus().peerId as string);


Same here and in other places with null checks

coder11 · 2022-05-11T09:47:23Z

src/__test__/integration/peer.spec.ts

-
-            // act
-            await peer.start({ connectTo: addr });
+            await peer.start({ connectTo: new Multiaddr(nodes[0].multiaddr) });


I think we should either use "AAA" pattern universally or not use it at all. At least in terms of pure unit tests.

IMO (esp, coming from backend / C# background) "AAA" thing is very neat. It forces you to think about the precise scope of the unit test.

Btw this test series can be written in table form. Seems like...

I haven't heard about AAA before. In my personal opinion adding comments doesn't make anything clearer, I accidentally removed them. But with tests it's almost always this pattern anyways, but without explicit comments. We can discuss if it's easier for you to read, maybe. I would suggest regarding this question to do whatever you want - I don't think consistency matters here. Some tests could have the comments some test could be ok without them and we always can add or remove them later

coder11 · 2022-05-11T09:51:48Z

src/__test__/integration/avm.spec.ts

-        if (peer) {
-            await peer.stop();
-        }
+    beforeAll(async () => {


We want beforeEach and afterEach here so that tests don't interfere with each other

I used beforeAll just so the tests run a bit faster. These test should not interfere with each other anyways. What do you think?

These tests connect to network and send particles. All this processing is happening inside the same peer. So in theory tests can interfere

Agreed. Will use beforeEach and afterEach just in case

coder11 · 2022-05-11T09:53:58Z

src/internal/utils.ts

-        };
-    };
-};
+export const MakeServiceCall =


To my taste the former is more explicit, thus easier to understand in a quick glance

I always remove the syntax that is not required if it doesn't harm readability. If you sure you want this reversed I can do it. It's all purely a taste thing after all. We could even decide and have a eslint rule for that so we never discuss this again

coder11 · 2022-05-11T09:57:17Z

src/internal/Particle.ts

-
-        return res;
+    constructor(
+        public id: string,


That's cool. Did not know TS can do that :)

coder11 · 2022-05-11T09:58:43Z

src/internal/Particle.ts

-    script: string;
-    signature: string;
-    data: Uint8Array;
+    signature?: string;


There will always be particle signature in future. It should be calculated from it's immutable part.

to make the signature not optional and retain the guaranties - it should be assigned in the constructor. If we can't do it now - I suggest we postpone it till later

I mean for now the correct situation we are in is that we don't always have the signature so it's better if we can see it in types. I will add TODO for that, ok?

Let's add a TODO. Sounds good

Turn on noImplicitAny and strictNullChecks

00b4067

shamsartem requested a review from coder11 May 10, 2022 09:19

coder11 approved these changes May 11, 2022

View reviewed changes

coder11 reviewed May 11, 2022

View reviewed changes

code review fixes

eea5ea8

shamsartem merged commit 5234ba2 into master May 12, 2022

shamsartem deleted the ts-refactor branch May 12, 2022 14:14

Uh oh!

Turn on noImplicitAny and strictNullChecks #153

Turn on noImplicitAny and strictNullChecks #153

Uh oh!

Conversation

shamsartem commented May 10, 2022

Uh oh!

coder11 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants