arangodb / arangojs

The official ArangoDB JavaScript driver.
https://arangodb.github.io/arangojs
Apache License 2.0
600 stars 106 forks source link

Type issue after updating to v8.4.0 #797

Closed hasithaweerasinghe closed 1 year ago

hasithaweerasinghe commented 1 year ago

Hey everyone,

I hope you're doing well. I've recently encountered a type-related issue after updating ArangoJS from version 8.0.0 to 8.4.0 in our project. This issue pertains to the changes in type definitions that seem to have caused a regression in some core functionalities.

export async function getUser(name:string) {

  const results = await db.query(
      aql`FOR u in User FILTER ${name} == u.name RETURN u`
  );

  return await results.next();

}

The return type of the above database query has been updated to (8.4.0) Promise<ArrayCursor<undefined>>, whereas in version 8.0.0, it was Promise<ArrayCursor<any>>. Although it is possible to specify a type for the AQL string handler, it appears that the default type of any is no longer functioning as expected.

export declare function aql<T = any>(templateStrings: TemplateStringsArray, ...args: AqlValue[]): GeneratedAqlQuery<T>;

Following image shows the VScode intellisense types:

image

Thanks in advance!

pluma4345 commented 1 year ago

This feels like it's a bug in TypeScript actually. You can get the correct behavior by splitting the code into two statements:

const query = aql`FOR u in User FILTER ${name} == u.name RETURN u`; // type: GeneratedAqlQuery<any>
const results = await db.query(query); // type: ArrayCursor<any>

I'm investigating what is triggering this unexpected behavior in TypeScript.

pluma4345 commented 1 year ago

With the help of @nikhil-varma we found multiple ways to force the intended behavior:

  1. Removing the GeneratedAqlQuery type so aql casts its return value to AqlQuery instead.
  2. Adding an overload to query that explicitly accepts a GeneratedAqlQuery argument.
  3. Changing the type marker from [type]?: T to [type]: T.
  4. Changing the type marker from [type]?: T to [type]?: T | any.

We went with the fourth solution as it forces the intended behavior without requiring major changes to the code or impacting backwards compatibility.

djanogly commented 1 year ago

Thank you @pluma4345 & @nikhil-varma - @hasithaweerasinghe is off today and back Monday, so will try these fixes then and report back.