[Draft RFC] Using Op.* as functions on top of keys

[ephys]

Feature Description

Is your feature request related to a problem? Please describe.

I've personally never liked the ergonomics of having to use sequelize operators as keys for an object.
The following code

const where = { id: { [Op.notIn]: blockList } };

feels easier to read when written like this:

const where = { id: Op.notIn(blockList) };

Describe the solution you'd like

We could make it possible to use Op.* as both object keys and functions using toPrimitive. That would make both snippets of code above valid.

Proof of concept:

const symbolNotIn = Symbol('notIn');

function notIn(val) {
  return { [symbolNotIn]: val };
}

notIn[Symbol.toPrimitive] = () => {
  return symbolNotIn;
}

console.log(notIn([1, 2]));        // output: { [Symbol(notIn)]: [1, 2] }
console.log({ [notIn]: [1, 2] });  // output: { [Symbol(notIn)]: [1, 2] }

export const Op = {
  // ...
  notIn,
  // ...
};

Why should this be in Sequelize

I believe this would make the usage of operators easier to read and understand.

Describe alternatives/workarounds you've considered

If using Symbol.toPrimitive feels too much like magic, those functions could be provided somewhere else. Sequelize.and, Sequelize.or, etc already exist.

Additional context

N/A

Feature Request Checklist

Is this feature 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

Would you be willing to implement this feature by submitting a Pull Request?

• 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.

[github-actions]

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. 🙂

[ephys]

still ready to work on this if interested :)

[github-actions]

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

[WikiRik]

Hi @ephys
Thanks for willing to work on this, just submit a PR and then a maintainer can look into it!

[ephys]

Still intending on working on this but replacing the symbols with functions+Symbol.toPrimitive could be a breaking change for some users.

Is the main branch considered v7 or should I wait?

[WikiRik]

We do not have a v7 branch yet, but that might be good to have. @sdepold do you agree?

EDIT: Would be good if the v7 branch only accepts PRs in TypeScript so we don't have to convert those later

[sdepold]

There we go https://github.com/sequelize/sequelize/tree/v7

[sdepold]

I'm not sure if we can have constraints on the language automatically somehow or if that is more a part of the review process

[sdepold]

We can even setup automated releases of a v7 alpha version to npm if you think that makes sense

[ephys]

Thanks! I think it's possible to add a check that ensures touched files do not end in .js, but it's easier to add a note in the PR guidelines I think.

[sdepold]

Are the PR guidelines taken from the target or the source branch? :D

[ephys]

I think from the target but only one way to find out :)

[sdepold]

I guess since we will remove JS code, having it in the documentation might be more useful. What do you think ? sequelize/sequelize#13747

[ephys]

I'd say a simple note in CONTRIBUTING.md should be enough

[ephys]

After working on a prototype, here are my findings:

TypeScript has very little support for Symbol.toPrimitive and while I did manage to make a prototype that can be used as both key and function call

it breaks the typing of WhereOperator because the symbols are not unique anymore

The following TypeScript issues are relevant:

• { [Symbol.toPrimitive]: () => Symbol }: support as index/property type microsoft/TypeScript#43428
• Improve support for Symbol.toPrimitive microsoft/TypeScript#4538
• Allow wrapped values to be used in place of primitives. microsoft/TypeScript#2361

I see three options going forward, none of which are particularly amazing:

• Break WhereOperator typing. Doing { [Op.eq]: value } would not be type safe anymore, in favor of Op.eq(value) being type checked. (a no-go imo)
• Wait until TypeScript has proper support for toPrimitive (likely to take years tbh).
• Expose these functions separately. OpFn.eq (function) & Op.eq (symbol). (OpFn being a placeholder until I find a better name).

[ephys]

Due to TypeScript not supporting Symbol.toPrimitive, maybe we could go the same route as Sequelize.and / Sequelize.or and expose a utility function for most / all operators:

import { notEqual } from 'sequelize';

User.findAll({
  where: { id: notEqual(5) }
});

That's a lot of extra exports with very generic names though :/

[sdepold]

In general I like the proposal but would maybe not expose it via sequelize but maybe add a layer or nesting?

Import { Operators} from sequelize

Const { notEqual}=Operators

?

[ephys]

I agree, but Operators is very close to Op even though one is a namespace for functions, and the other one for symbols. I fear people might start using Operators as keys by accident

If we expose something like Operators, I think we should move Sequelize.and & Sequelize.or over to Operators too

This issue is frustrating. I have a solution that is almost working but it causes typescript to stop reporting errors with the current way of using operators:

[ephys]

I'm transfering RFCs to the main repo to gather feedback from users of the library, but I won't migrate this one because I'm not happy with it yet

[WikiRik]

Feel free to make a new issue that supersedes this one to have a cleaner discussion