[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[]

[runspired]

I spent some time seeing if I could type strings nicely: broad strokes its not possible unless TypeScript implements something that lets us write functions that take multiple generics and infer any remaining generics that aren't supplied (See the very long thread here for history of debate of adding that feature: microsoft/TypeScript#10571)

There's a poor work around we could do with very negative perf ramifications where we take the union of includes and manually build out N loose combinations (loose meaning we allow repeats). Doing this we'd be able to offer includes with type-ahead up to length N in a typed way, and if a user exceeds that length they'd need to either ts-expect-error or use an array.

The performance makes this option a non-starter: we already arbitrarily limit relationship inclusion depth to a maximum of 3 path segments (e.g. <record>.foo.bar.baz, but even still we regularly encounter valid path unions hundreds to thousands of strings long.

Given a union of 100 valid paths, an approach like below with N of 3 results in 100*100*100 + 100*100 + 100 = 1,010,100 combinations.

type ValidPaths = ${IncludeUnion},${IncludeUnion},${IncludeUnion} | ${IncludeUnion},${IncludeUnion} | IncludeUnion

[mkszepp]

I think, we can users teach to pass string[] instead of string type, like it is now... i prefer also passing a array of string, specially when include paths are long (for a lot of includes we have created always an array and than used join in js before passing to ember data).

Our app is atm using the legacy code and in all points in which we have used includes, we have added a ts-expect-error to skip ts error, because the legacy code isn't working correctly when you pass an string array like described.

I think that is my (and also for other users which want migrate to typescript there app) the bigger issue, because we have trusted all docs with strings and the old code part isn't working with string array.
It would be nice to fix that issue, so peoples can easier and step by step migrate to TS and than to the request manager. This was our plan, because our app is not so small and making to much changes in one time is to much for us