Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strict type decleration for PropertyName causing tsc compile error #2229

Closed
JmsPae opened this issue Aug 11, 2022 · 3 comments · Fixed by #2343
Closed

Strict type decleration for PropertyName causing tsc compile error #2229

JmsPae opened this issue Aug 11, 2022 · 3 comments · Fixed by #2343
Labels

Comments

@JmsPae
Copy link

JmsPae commented Aug 11, 2022

I'm working on a project using typescript and created an index using json. Each json object has a unix timestamp, and I wanted to filter as well as sort objects with timestamps from the last 5 mins which led to the code below;

redis.ft.search('vehicleIdx', '@timestamp:[${Math.floor(Date.now() / 1000) - 300}, +inf]', {SORTBY: {BY: 'timestamp', DIRECTION: 'DESC'}, LIMIT: {from: 0, size: 100}})

At least when compiling from ts this will cause a compilation error due to the lack of a @ or $. prefix before 'timestamp' as the SORTBY argument. This can be fixed quite easily by packages/search/lib/commands/index.ts line 126 to

export type PropertyName =`${string}`;

... And it works for me just as intended.

I'm unfamiliar with the codebase and started working with redis the other night so I wonder if there would be any unintended consequences to such a change.

Environment:

  • Node.js Version: 18.7.0
  • Redis Server Version: 7.0.4
  • Node Redis Version: 4.2.0
  • Platform: Ubuntu 22.04
@JmsPae JmsPae added the Bug label Aug 11, 2022
@JmsPae
Copy link
Author

JmsPae commented Aug 11, 2022

For a little more context;

Index creation: FT.CREATE vehicleIdx ON JSON PREFIX 1 vehicles:data: SCHEMA $.timestamp AS timestamp NUMERIC SORTABLE

roughly equivalent ft.search: FT.SEARCH vehicleIdx "@timestamp:[0, +inf]" SORTBY "timestamp" DESC

@JmsPae JmsPae changed the title Type decleration for PropertyName (for use in SortByProperty) in ft.search causing tsc compile error, despite equivelent working in redis-cli strict type decleration for PropertyName (for use in SortByProperty) in ft.search causing tsc compile error Aug 11, 2022
@JmsPae JmsPae changed the title strict type decleration for PropertyName (for use in SortByProperty) in ft.search causing tsc compile error strict type decleration for PropertyName causing tsc compile error Aug 11, 2022
@JmsPae JmsPae changed the title strict type decleration for PropertyName causing tsc compile error Strict type decleration for PropertyName causing tsc compile error Aug 11, 2022
@VladimirChuprazov
Copy link
Contributor

Have the same problem.

SortByProperty type accepts only PropertyName type, but should accept just string.

// packages/search/lib/commands/index.ts:137
export type PropertyName = `${'@' | '$.'}${string}`;

export type SortByProperty = PropertyName | {
    BY: PropertyName;
    DIRECTION?: 'ASC' | 'DESC';
};

Even in examples it is used without $. or @
https://github.com/redis/node-redis/blob/master/examples/search-hashes.js#L51

Environment:

  • Node Redis Version: "4.5.1"

@leibale leibale linked a pull request Dec 14, 2022 that will close this issue
1 task
@leibale
Copy link
Contributor

leibale commented Dec 14, 2022

Fixed in #2343. I'll comment here again once it's released

@leibale leibale closed this as completed Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants