[5.4.0-alpha.91] Incorrect/Missing typings

[mkszepp]

I have recently updated in our app the native type packages from 5.4.0-alpha.49 to 5.4.0-alpha.91

After updating we have seen that there already fixed other incorrect types (thanks for that), but other types are now incorrect or maybe other improvements are bringing now incorrect TS errors

1. The option include was defined as array. it should be a string, otherwise the JSONAPI call is incorrect. Passing as array generates url &include[]=pets, which is not correct. demo - JSON:API Specs. This error is in findAll, findRecord, query... (i think in all legacy functions)

const users = await this.store.findAll<UserModel>('user', {
    // Error "Type 'string' is not assignable to type 'undefined[]'.ts(2322)"
    include: 'pets',
  });

2. When we a findAll returns a list of models and we take for example the first to make additional calls like findRecord, we see error Error Argument of type 'string | null' is not assignable to parameter of type 'string | number'. Type 'null' is not assignable to type 'string | number'.ts(2345) demo
   Expected: id should be string | number, instead of string | null

user = await this.store.findRecord<UserModel>('user', firstUser.id, {
        include: 'pets',
      });

3. We have used in some models this.constructor.relationshipsByName;. In this case we get error "Property 'relationshipsByName' does not exist on type 'Function'.". This works in JS... but typing looks like incorrect or missing see

 relationshipNames() {
    // Error "Property 'relationshipsByName' does not exist on type 'Function'."
    return this.constructor.relationshipsByName;
  }

All TS errors you can find in this repo: https://github.com/mkszepp/ember-data-ts-errors

[runspired]

these are all expected

• the correct type for includes is array, that string works has been a (happy) accident. I'll leave this ticket open because it should generate the proper include string from an array, and if it is not I'd like to know more about the setup to either confirm its an issue with just your app or if we have something we should fix.
• the correct type for id is string | null unless using separate types for read vs create vs edit. This is especially true for a findAll where you are not guaranteed to get only remotely available records in the response. This means you need to ensure its just a string before invoking findRecord.
• TypeScript does not allow typing constructors, so constructor being untyped is unsurprising, you'd need to cast.

[mkszepp]

• the correct type for includes is array, that string works has been a (happy) accident. I'll leave this ticket open because it should generate the proper include string from an array, and if it is not I'd like to know more about the setup to either confirm its an issue with just your app or if we have something we should fix.

Yes, when array will be joined to a string its okay for me.
Wow, this means that code examples in repo docs are not correct or? For example:

data/packages/store/src/-private/store-service.ts

Line 1710 in 4badefd

return this.store.findAll('post', { include: 'comments,comments.author' });

Also this docs are saying that we should pass as string https://guides.emberjs.com/release/models/relationships/#toc_retrieving-related-records

• the correct type for id is string | null unless using separate types for read vs create vs edit. This is especially true for a findAll where you are not guaranteed to get only remotely available records in the response. This means you need to ensure its just a string before invoking findRecord.

Okay thanks

• TypeScript does not allow typing constructors, so constructor being untyped is unsurprising, you'd need to cast.

Thanks for info

[runspired]

We do support string (we test, don't error for it), I guess I should just clarify that's not the intended way to use it and it was sort of an accident that it became possible (and got documented). Almost all of the code related to it treats it as an array which just happens to work because string and array APIs share a lot of overlap. We did eventually special case some handling of it as a string, typically to split it back into an array.

For typescript, we are only going to type the array story, as the string based story is both largely un-typeable and makes a lot of the features around include not work as seamlessly.

Also this docs are saying that we should pass as string

unfortunately the guides are a separate project the data team doesn't directly maintain, though we have been working to clean them up. @Baltazore has been doing work in there recently and could probably fix that quickly.

[mkszepp]

Thank you for the detailed explanation. It would also be good to get a codemode for the migration, because that would help everyone to correct existing code in project (whether TS or JS).

Maybe the team behind lint rules, should also provide a lint rule for it, so that one day you can get rid of wrong code and support only string[]