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

fix(types): fix typing errors and limitations #799

Merged
merged 1 commit into from Sep 8, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions types/index.d.ts
Expand Up @@ -103,7 +103,7 @@ interface RecordsSerialized {
}

export class AbstractRecordTool<M extends Sequelize.Model> {
constructor(model: Sequelize.ModelCtor<M>, user: User, query: Record<string, any>)
constructor(model: Sequelize.ModelCtor<M>, user: User, query: Query)
serialize(records: M | M[]): Promise<RecordsSerialized>;
}

Expand Down Expand Up @@ -144,7 +144,7 @@ export class RecordsRemover<M extends Sequelize.Model> extends AbstractRecordToo

export class RecordSerializer {
constructor(model: { name: string } | Sequelize.ModelCtor<any>, user?: User, query?: Query);
serialize(records: Record<string, any> | Record<string, any>[]): Promise<RecordsSerialized>;
serialize(records: Record<string, any> | Record<string, any>[], meta: Record<string, any>): Promise<RecordsSerialized>;
}

// Everything related to Forest permissions
Expand Down Expand Up @@ -205,7 +205,7 @@ export interface Query {
search?: string;
fields?: {[key: string]: string};
sort?: string;
filters?: Filter|AggregatedFilters;
filters?: string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hurts me a little bit. By integrating TypeScript in our repos, I strove to provide clear guidance and definition on our API to customers. By using this, this makes the Query.filters attribute impossible to use without strong code introspection.

Do we plan to work on this and change this to a true filter ? (Which already defined and available 😢)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan to work on this and change this to a true filter ? (Which already defined and available 😢)

The API that is exposed to our customers is low level (customers write routes, middlewares etc), and this type represents the querystring, fresh from express. That's why filters are strings.

At this point it cannot be decoded by us

By integrating TypeScript in our repos, I strove to provide clear guidance and definition on our API to customers
Do we plan to work on this and change this to a true filter ? (Which already defined and available 😢)

IMHO, the problem is not the typings, it's the API itself which would gain from being higher level.

I'm a strong proponent for breaking changes in the agents APIs so that customers have a higher level API for next major.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a place where we list stuff to be done in the next major version ? If this is the case, let's log this subject in this place to keep a track on, and validate this PR 👍

page?: Page;
searchExtended?: string;
}
Expand Down Expand Up @@ -272,6 +272,7 @@ export interface SmartActionField {
defaultValue?: any,
reference?: string,
hook?: string,
widget?: string;
}

export interface SmartActionHookField extends SmartActionField {
Expand Down