Add typing for RawBinding and ValueMap

[cham11ng]

Add typing for RawBinding and ValueMap

This was the error I was facing. Since, its type was not exported I had to include it in my codebase.

[ts]
Argument of type 'object' is not assignable to parameter of type '(string | number | boolean | Raw | QueryBuilder | string[] | number[] | Date | Date[] | boolean[] | Buffer)[] | ValueMap'.
  Type 'object' is not assignable to type 'ValueMap'.
    Index signature is missing in type '{}'. [2345]
(parameter) params: object

I've added RawBinding and ValueMap.

[arenddeboer]

Merging this would be highly appreciated!

[kibertoad]

@arenddeboer Could you please test updated binding locally to confirm they are indeed correct? I am not familiar enough with this part to merge this confidently.

[kibertoad]

I can understand RawBinding being added, but aren't Value and QueryBuilder valid params as well? Shouldn't they be also kept?

[arenddeboer]

I agree, I see no need to remove them.

[arenddeboer]

@kibertoad I have tested the pull request as is and this indeed solves the issue with the bindings.
I'm no expert on TypeScript though, so if anyone else can comment on this please do.

[kibertoad]

@arenddeboer Wait, I only now noticed - does this PR even do anything? Isn't RawBinding that it add is exactly same thing as (Value | QueryBuilder)[] that it replaces?

[arenddeboer]

Sorry for the delay.
The main problem for me was being unable to pass a bindings object, because ValueMap is an non-exported type. Making it an interface under the Knex namespace allows us to do something like:

// index.d.ts inside namespace Knex 
interface ValueMap {
    [key: string]: Value | Knex.QueryBuilder | undefined;
  }
// custom code.ts:
/** knex js bindings for my query */
    interface IBindings extends ValueMap {
      siteId: number;
      languageId: number;
      country?: string;
    }
    const bindings: IBindings = 
      siteId,
      languageId,
    };
knex.raw(myQuery, bindings)

So to get bindings working like this, one does not have to change RawBinding But I do not know which other problems @cham11ng ran into to warrant the other changes.

[elhigu]

This makes me wonder... now that we have typings in knex repo it would be possible to actually write tests for typings. Not saying that it needs to be done in this PR.

[arenddeboer]

I agree and was considering mentioning this in my last reply. Not sure what the best way forward is for Typescript tests though, but it makes a lot of sense and would probably help reason about current and future changes.

[kibertoad]

@lorefnon Could you please take a look if this change still makes sense after all the latest changes?

[lorefnon]

The only change here is that RawBinding and ValueMap (now ValueDict) are being exposed. Remaining changes are no-op.

I may not have the full clarity here, but I don't think this is (and ever was) very useful because typescript's inference should have done the right thing (even with the bindings we had before). It is likely that in the application there is somewhere an explicit type annotation (as object) causing it to fail which can be removed.

For example:

const bindings: object = {id: 1};

knex.raw('select * from "users" where "id" = ?', bindings);
// This fails because object is not assignable to ValueMap.

But simply removing that type annotation, will fix this:

const bindings = {id: 1};

knex.raw('select * from "users" where "id" = ?', bindings); 
// { id: number } is implicitly assignable to ValueMap

Nevertheless, if this makes someone's life simpler I can rebase this change against current master.

[kibertoad]

@lorefnon No need to rebase per se, probably would be much easier to just do sensible changes separately. Just exposing ValueDict seems like a reasonable change to allow people type their code more explicitly (otherwise they would need to introduce some local types that would then duck-type to ValueDict on compilation). If you are still doing any changes to typings, could you include that part?

[lorefnon]

As per #3036 (comment) I have exposed this and some other utility types in #3211.

@cham11ng I am adding you as a co-author in this PR so your contribution wouldn't get lost. Please let me know if you are not ok with this.

[cham11ng]

@lorefnon It's okay

[kibertoad]

Closing this in favour of #3211