refactor(types): enable noImplicitAny for transaction.ts & request.ts

[josecervino]

Fixes #252 (it's a good idea to open an issue first for discussion)

• Tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

[JustinBeckwith]

Just a first pass. Will come back around with more detail after feedback is addressed :)

[josecervino]

@JustinBeckwith Thanks for taking the time out to look this over! I know it wasn't easy with so many fractured commits, I'm going to be making it a habit to keep it to one commit per issue. In general, a lot of the convoluted, unnecessary logic was created to pass the linter/formatter. I'll fix it asap

[JustinBeckwith]

@JCIV-SE this looks like it needs a little love :) Is this going to get a look soon?

[josecervino]

@JustinBeckwith Yup! It's next on the list 👍

[JustinBeckwith]

No need to explicitly return void here

[josecervino]

Agreed, but I added the void to solve this error:

Property 'delete' in type 'Transaction' is not assignable to the same property in base type 'DatastoreRequest'.
  Type '(entities: any) => void' is not assignable to type '{ (keys: any): Promise<CommitCallback>; (keys: any, callback: CommitCallback): void; (keys: any, gaxOptions: CallOptions, callback: CommitCallback): void; }'.
    Type 'void' is not assignable to type 'Promise<CommitCallback>'.

delete in transaction.ts has a void return annotation, and when changed to the same Promise there's an error asking explicitly for an any or void return annotation. I kept the promise to follow the "no callback = return promise" rule.

Should I make them all void and remove the promise return annotation from the method definition? (if all method overloads have the same return annotation, do we really need the overloads?)

[JustinBeckwith]

Well hold on nah - what does CommitCallback have in it's type? Is it a callback function type? The answer here may be to return Promise<void>.

[josecervino]

Yeah, CommitCallback is a callback function type. I also tried Promise<void> but I'm getting the same errors. I'll ask around to see if we can come up with something using feedback from an IDE

[JustinBeckwith]

Returning the Entity or a Stream doesn't feel quite right. What am I missin here?

[josecervino]

I admit the reasoning isn't 100% here. get relies on createReadStream which returns a stream, but get is expecting something iterator-related:

Type 'Stream' must have a '[Symbol.iterator]()' method that returns an iterator.

826       const [[deletedEntity], [fetchedEntity]] = await Promise.all([

Changing it to Promise<Entity> works well - lmk your take on it :)

[JustinBeckwith]

We aren't doing this null check in other places, so I don't think it's entirely needed.

[josecervino]

Yeah I agree, I'd prefer the consistency - the problem is that typeof null evalutes to 'object' in JS (thank you C) and a test uses null to trigger options be set to an object literal:

it('should default options to an object', done => {
        request.get(keys, null, err => {  // 2nd arg 'null' is what's being used to trigger the options object correction logic
          assert.ifError(err);

          const spy = request.createReadStream.getCall(0);
          assert.deepStrictEqual(spy.args[1], {});
          done();
        });
      });

Was thinking we should actually add the null check in other places because this doesn't tend to be accounted for

[JustinBeckwith]

I don't think we need the void here

[josecervino]

Getting the same error/problem as delete - I'll figure something out