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

Typescript queries with raw: true should not return model instances #12575

Closed
2 of 7 tasks
JacobLey opened this issue Jul 30, 2020 · 6 comments · Fixed by #12921
Closed
2 of 7 tasks

Typescript queries with raw: true should not return model instances #12575

JacobLey opened this issue Jul 30, 2020 · 6 comments · Fixed by #12921
Labels
released on @v7 type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Comments

@JacobLey
Copy link
Contributor

JacobLey commented Jul 30, 2020

Issue Description

Currently when running a query with a model in Typescript, the result(s) are instances of the model.
e.g.

import { Model } from 'sequelize';

class Foo extends Model {};

async () => {
    // Foo.findAll() returns Promise<Foo[]>
    const foos: Foo[] = await Foo.findAll();
};

This is expected and ideal behavior.
However when running a query with raw: true (https://sequelize.org/master/manual/raw-queries.html) the response is very much not an instance of the model (what it is is up for discussion in this issue).

The typing for queries that support raw should support returning non-model responses.

async () => {
    const foos: = await Foo.findAll({
        attributes: [[fn('RAND'), 'random']],
        raw: true,
    });
    
    foos.forEach(foo => {
        // Typescript fails on `random` is not on `Foo`
        console.log(foo.random);
    });
};

What was unclear/insufficient/not covered in the documentation

Presently, all Model query methods that support {raw: true} still declare they return the model instance, when raw queries are explicitly not instances.

So regular model attributes like get are not even available (which are used as a workaround for custom attributes on non-raw results).

If possible: Provide some suggestion on how we can enhance the docs

Allow type parameters on methods that support raw: true to return a custom type, defaulting to the models raw attributes.

As an example, below is the existing definition for findAll: https://github.com/sequelize/sequelize/blob/v6/types/lib/model.d.ts#L1826

  public static findAll<M extends Model>(
    this: ModelStatic<M>,
    options?: FindOptions<M['_attributes']>): Promise<M[]>;

If we added another method definition that explicitly declared when raw: true to return a type parameter, it could correctly handle typing raw queries:

public static findAll<M extends Model>(
    this: ModelStatic<M>,
    options?: FindOptions<M['_attributes']> & { raw?: false }): Promise<M[]>;
  public static findAll<M extends Model, R extends any = M['_attributes']>(
    this: ModelStatic<M>,
    options?: FindOptions<M['_attributes']> & { raw: true }): Promise<R[]>;

The example query above could now be supported as:

Foo.findAll<Foo, { random: number }>({
   attributes: [fn('RAND'), 'random'],
   raw: true,
})

Additional context

If there is another known method (or suggestions) for typing raw queries I'd appreciate the advice.

I know it is possible to just run the query as raw: false and use getters to load the custom attributes, but I believe raw queries serve their purpose when the extra noise of model instances is unnecessary. Not to mention the current typing is objectively wrong in this case.

Issue Template Checklist

Is this issue dialect-specific?

  • No. This issue is relevant to Sequelize as a whole.
  • Yes. This issue only applies to the following dialect(s): XXX, YYY, ZZZ
  • I don't know.

Would you be willing to resolve this issue by submitting a Pull Request?

Happy to make an actual PR with my suggested findAll() as well as a couple others (possibly findOne + findByPk?) if the solution sounds correct

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I don't know how to start, I would need guidance.
  • No, I don't have the time, although I believe I could do it if I had the time...
  • No, I don't have the time and I wouldn't even know how to start.
@JacobLey JacobLey added the type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference. label Jul 30, 2020
@duylddev
Copy link

So, there's no solution for this, any idea?

@JacobLey
Copy link
Contributor Author

JacobLey commented Dec 19, 2020

I realized I never made an official PR, so I added that at least. #12921

My workaround is in some model definition file write:

import Seq from 'sequelize';

declare module 'sequelize' {

    export abstract class ModelRaw<T, T2> extends Seq.Model<T, T2> {

        /**
         * @override
         *
         * @see {@link https://github.com/sequelize/sequelize/issues/12575}
         */
        public static findAll<M extends Seq.Model>(
            this: ModelStatic<M>,
            options?: FindOptions<M['_attributes']> & { raw?: false }): Promise<M[]>;
        public static findAll<M extends Seq.Model, R extends any = M['_attributes']>(
            this: ModelStatic<M>,
            options?: FindOptions<M['_attributes']> & { raw: true }): Promise<R[]>;

        /**
         * @override
         *
         * @see {@link https://github.com/sequelize/sequelize/issues/12575}
         */
        public static findOne<M extends Seq.Model>(
            this: ModelStatic<M>,
            options: NonNullFindOptions<M['_attributes']> & { raw?: false }
        ): Promise<M>;
        public static findOne<M extends Seq.Model, R extends any = M['_attributes']>(
            this: ModelStatic<M>,
            options: NonNullFindOptions<M['_attributes']> & { raw: true }
        ): Promise<R>;
        public static findOne<M extends Seq.Model>(
            this: ModelStatic<M>,
            options?: FindOptions<M['_attributes']> & { raw?: false }
        ): Promise<M | null>;
        public static findOne<M extends Seq.Model, R extends any = M['_attributes']>(
            this: ModelStatic<M>,
            options?: FindOptions<M['_attributes']> & { raw: true }
        ): Promise<R | null>;
    }
}

class Foo extends Seq['Model' as 'ModelRaw'] { ... }

Its a hack, but it has worked well enough for me.

@papb papb added type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. and removed type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference. labels Mar 22, 2021
@vikasg603
Copy link
Contributor

Any update on this issue?

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has been open for 7 days without activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

@github-actions github-actions bot added the stale label Oct 31, 2021
@Darkle
Copy link

Darkle commented Oct 31, 2021

Yes this is still an issue.

@github-actions github-actions bot removed the stale label Nov 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2022

🎉 This issue has been resolved in version 7.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants