diegomvh / angular-odata

Client side OData typescript library for Angular
https://www.npmjs.com/package/angular-odata
MIT License
50 stars 14 forks source link

OrderBy url formed incorrectly #5

Closed kevindstanley1988 closed 4 years ago

kevindstanley1988 commented 4 years ago

As of 0.8 the strict typing is great. I did start to get a couple of errors around my application when I upgraded to it though, focused around the OrderBy

The old way I would do orderby with ascending or descending properties.

const service = this.showService.entities();
    service.orderBy(["DateModified desc"]);
    service.get().subscribe(result => {
        ....
    });

0.7.1 url that was working with the above code https://localhost:44317/odata/Shows?$orderby=DateModified%20desc

The new type checking is not allowing for the 'desc' part of the string, it allows them to be split like this ['PROPERTY','ORDER'].

const service = this.showService.entities();
    service.orderBy(["DateModified", "desc"]);
    // service.filter(applyFilter);
    service.get().subscribe(result => {
        ....
    });

Problem Splitting the property and the order seems to clear up the type checking, only problem is the url is now formed incorrectly. the ,desc is the culprit here, it is acting as any other property would in OData orderby. https://localhost:44317/odata/Shows?$orderby=DateModified,desc

Proposals I tried to look into modifying the OrderBy type definition to allow for concatenated definitions and couldn't come up with that typescript would allow. the idea was to get it to only allow you to enter 1 of 3 ways.

  1. The Property string by itself Ex: ["DateModified"]
  2. The Property string + " asc" Ex: ["DateModified asc"]
  3. The Property string + " desc" Ex: ["DateModified desc"]

I cannot seem to come up with a type that will allow me to do something like [keyof T + " asc" | " desc"]

I also tried tuple concatenation, but I cannot get it to come out as a type, only a variable.

The only other solution I can think of is to keep the type definition the way it is and change the way the url is formed. Basically read for the 'asc' or 'desc' properties and created spaces instead of commas. I haven't looked into that part of the code yet though.

Workaround For the time being you can just ignore type checking on the order by with orderBy: ['DateModified asc'] as any

diegomvh commented 4 years ago

the main intention for the orderBy was:

orderBy (string) where string is any string

orderBy ([keyof T, ...]) an array of entity fields (properties), where asc is the default value for all fields

orderBy ([[keyof T, 'asc' | 'desc'], ...]) an array of tuples (n = 2) where each tuple has the name of the property and the order asc or desc

PD: The project is lazy of documentation :(

kevindstanley1988 commented 4 years ago

That makes total sense, I didn't try the nested array, that completely works how I was expecting and forms the url correctly. Thank you!

It seems I may be one of the only other people using this library actively, so I am not expecting a fully documented app by any means - I am trying to read through your library code before posting 'issues' (not that this was actually an issue), I typically figure out what I need, this one just had me stuck. I still think this might be the most structured OData library for angular so far, especially in combination with the generator. If I see things I can fix I will definitely be doing that and send them your way.