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

Suggestion: Remove all aliases in v5 #9372

Closed
SimonSchick opened this Issue Apr 29, 2018 · 14 comments

Comments

3 participants
@SimonSchick
Copy link
Contributor

SimonSchick commented Apr 29, 2018

Not sure if I'm too late here but over the years Sequelize has create many many aliases functions. I think it would be a good idea to remove the aliases with the next major release.

  • find vs. findOne
  • findById vs. findByPrimary
  • afterBulkDestroy vs afterBulkDelete
  • addHook v. hook
  • VIRTUAL vs NONE

Edit: Formatting

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Apr 30, 2018

I have edited your post for better readability.

Good idea overall, we need to deprecate duplicate cases first, then remove them from master. This change should be easy to implement in small PRs

@sushantdhiman sushantdhiman added this to the v5 milestone Apr 30, 2018

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Apr 30, 2018

@sunshinewyin the ones Iisted where just examples, there are quite a few more.

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Apr 30, 2018

@sushantdhiman I'll see if I can do that.

@SimonSchick SimonSchick referenced this issue Apr 30, 2018

Closed

chore(*): mark aliases as deprecated #9374

4 of 5 tasks complete
@frlinw

This comment has been minimized.

Copy link
Contributor

frlinw commented Sep 14, 2018

As requested by @sushantdhiman

Final

Remove Keep
insertOrUpdate upsert ✔️
findById, findByPrimary findByPk ✔️
find findOne ✔️
findAndCount findAndCountAll ✔️
findOrInitialize findOrBuild ✔️
beforeConnection beforeConnect ✔️
[after,before]BulkDelete [after,before]BulkDestroy ✔️
[after,before]Delete [after,before]Destroy ✔️
hook addHook ✔️
updateAttributes update ✔️
DOUBLE PRECISION DOUBLE
NUMERIC DECIMAL
NONE VIRTUAL ✔️
asIs literal ✔️
Sequelize.condition Sequelize.where ✔️
Sequelize.prototype.Utils Sequelize.Utils ✔️
Sequelize.prototype.Promise Sequelize.Promise ✔️
Sequelize.prototype.TableHints Sequelize.TableHints ✔️
Sequelize.prototype.Op Sequelize.Op ✔️
Sequelize.prototype.Transaction Sequelize.Transaction ✔️
Sequelize.prototype.Model Sequelize.Model ✔️
Sequelize.prototype.Deferrable Sequelize.Deferrable ✔️
Sequelize.prototype.Error Sequelize.Error ✔️
Sequelize.prototype[error] Sequelize[error] ✔️

Question

Should we add deprecation message or just add the migration table in the changelog ?

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Sep 14, 2018

Thanks @frlinw , A few changes

  1. Keep both hasHooks/hasHook.
  2. upsert is well known name so we can keep this
  3. findByPk suggestion accepted

Should we add deprecation message or just add the migration table in the changelog ?

I can take care of deprecating them from v4 once these changes land to master

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Sep 14, 2018

Yea sorry, been super busy, cudos to @frlinw for taking over.

@frlinw

This comment has been minimized.

Copy link
Contributor

frlinw commented Sep 14, 2018

I started to work on it. I'm doing some reorg for imports (require) in test files because it's a mess, will make a PR after that.

@frlinw

This comment has been minimized.

Copy link
Contributor

frlinw commented Sep 15, 2018

beforeConnection added to the list, it's not used internally and it's the last hook alias

@frlinw frlinw referenced this issue Sep 18, 2018

Merged

Aliases deletion #9933

4 of 4 tasks complete
@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Sep 19, 2018

Implemented in #9933

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Oct 19, 2018

Hey, what about all the prototype aliases eg.

Sequelize.prototype.fn = Sequelize.fn;
Sequelize.prototype.col = Sequelize.col;
Sequelize.prototype.cast = Sequelize.cast;
Sequelize.prototype.literal = Sequelize.literal;
Sequelize.prototype.and = Sequelize.and;
Sequelize.prototype.or = Sequelize.or;
Sequelize.prototype.json = Sequelize.json;
Sequelize.prototype.where = Sequelize.where;
Sequelize.prototype.validate = Sequelize.prototype.authenticate;

and more?

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Oct 20, 2018

They are ok for now

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Oct 20, 2018

One more, what about the new DATATYPE() vs. DATETYPE() calls?

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Oct 21, 2018

new DATETYPE() is sure more verbose than DATETYPE, I prefer short functional syntax in this case. Once I tried to convert datatypes to classes but stopped just because of this issue

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Nov 17, 2018

Another alias I came across is Transaction.LOCK vs. transactionInstance.LOCK, all other enums like that are static, why is that one available on the instance as well?

Also at the same time that section should be optimized, accessing enums shouldn't invoke object creation, we should just Object.freeze and assign it directly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment