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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider making allowNull: false the new default #14260

Open
ephys opened this issue Mar 18, 2022 · 5 comments
Open

Consider making allowNull: false the new default #14260

ephys opened this issue Mar 18, 2022 · 5 comments
Assignees
Labels
type: feature For issues and PRs. For new features. Never breaking changes.

Comments

@ephys
Copy link
Member

ephys commented Mar 18, 2022

I don't think everyone is going to agree with me on this (feel free to express your opinion with the 馃憥 and 馃憤 reactions), but I think that nowadays we tend to be a lot stricter regarding what can and cannot be null.

My main reason for this change is that it would be better aligned with TypeScript. You'd only specify allowNull if you also made your type nullable:

class User extends Model {
  firstName: string,
}

User.init({ firstName: string });

// &

class User extends Model {
  firstName: string | null,
}

User.init({ firstName: { type: string, allowNull: true } });

But I also think that in most cases, it's safer to make the application crash when null accidentally got in a field that was accidentally left nullable, than having incomplete data in the database.

To ease migration, I would also add an option in the Sequelize constructor to choose whether attributes are nullable by default .


TypeORM made this change a few years back, I think we could too.

Past issues related to this:

@ephys ephys added the type: feature For issues and PRs. For new features. Never breaking changes. label Mar 18, 2022
@ephys ephys self-assigned this Mar 18, 2022
@WikiRik
Copy link
Member

WikiRik commented Apr 4, 2022

Would be big breaking change, but I think this is for the best. Good idea to add an option in the constructor so if people want to keep the default nullable they can

@javiermrz
Copy link

Hi! Is there any update on this?

@ephys
Copy link
Member Author

ephys commented Jan 14, 2023

It's currently blocked by #14682, which is itself blocked by #14715, which is itself blocked by #14687, which is itself blocked by an issue with describeTable we need to resolve

We'll get there eventually :/

@benasher44
Copy link

Hyped for this!

@fpirsch
Copy link

fpirsch commented Dec 19, 2023

Columns are nullable by default in databases. Making the opposite choice in the orm would be weird.
Why align with TypeScript ? Remember GraphQL fields are also nullable by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature For issues and PRs. For new features. Never breaking changes.
Projects
Status: No status
Development

No branches or pull requests

5 participants