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

chore(data-types): move to classes #10495

Merged

Conversation

SimonSchick
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

  • Moves all datatypes to be classes while retaining 100% backwards compatibility.
  • No more prototype madness
  • Removed lots of duplicate code
  • Code is much cleaner.

Remaining:

  • There are 4 questions:
    • Why does postgres alter base types?
    • Should there be an intermediate class for floating point data types to avoid more prototype patching?
    • The compatibility layer uses a Proxy which isn't necessarily very performant but should have no real impact on performance.
    • Should inferNew be exposed (as in the new docs)?

@SimonSchick
Copy link
Contributor Author

Arg, latest change caused a circular dependency, fixing..

@SimonSchick
Copy link
Contributor Author

@sushantdhiman I cannot repro these errors about toString not being generic, no idea what's going on here...

@codecov
Copy link

codecov bot commented Mar 2, 2019

Codecov Report

Merging #10495 into master will increase coverage by 0.01%.
The diff coverage is 94.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10495      +/-   ##
==========================================
+ Coverage   96.14%   96.16%   +0.01%     
==========================================
  Files          92       92              
  Lines        9501     9022     -479     
==========================================
- Hits         9135     8676     -459     
+ Misses        366      346      -20
Impacted Files Coverage Δ
lib/utils/implicitNew.js 100% <100%> (ø)
lib/utils.js 98.36% <100%> (ø) ⬆️
lib/dialects/mysql/data-types.js 98.43% <100%> (+2.35%) ⬆️
lib/dialects/mssql/data-types.js 100% <100%> (+2.06%) ⬆️
lib/dialects/sqlite/data-types.js 100% <100%> (+3.16%) ⬆️
lib/data-types.js 91.25% <90.47%> (-3.13%) ⬇️
lib/dialects/postgres/data-types.js 96.26% <95.51%> (-1.46%) ⬇️
lib/dialects/mariadb/data-types.js 98.03% <96%> (+9.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5c0d78...3d706b5. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 2, 2019

Codecov Report

Merging #10495 into master will increase coverage by 2.41%.
The diff coverage is 94.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10495      +/-   ##
==========================================
+ Coverage   93.76%   96.17%   +2.41%     
==========================================
  Files          92       92              
  Lines        9508     9026     -482     
==========================================
- Hits         8915     8681     -234     
+ Misses        593      345     -248
Impacted Files Coverage Δ
lib/utils/classToInvokable.js 100% <100%> (ø)
lib/utils.js 98.34% <100%> (-0.02%) ⬇️
lib/dialects/mysql/data-types.js 98.43% <100%> (+48.43%) ⬆️
lib/dialects/mssql/data-types.js 100% <100%> (+2.06%) ⬆️
lib/dialects/sqlite/data-types.js 100% <100%> (+3.16%) ⬆️
lib/data-types.js 91.25% <90.47%> (-3.13%) ⬇️
lib/dialects/postgres/data-types.js 96.26% <95.51%> (-1.46%) ⬇️
lib/dialects/mariadb/data-types.js 98.03% <96%> (+9.37%) ⬆️
lib/dialects/abstract/query-generator.js 97.57% <0%> (+0.26%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ccbb1e...e56256b. Read the comment docs.

@SimonSchick
Copy link
Contributor Author

@sushantdhiman works now

@SimonSchick
Copy link
Contributor Author

SimonSchick commented Mar 4, 2019

Please note there is a single potentially 'breaking' change:
DataType.toString() // static call will now yield the name of the DataType rather than the class definition, this seems to be a technical limitation of Proxy, I hope that's ok.

To clarify: We can change the return value, we just need to ensure it's unique for every datatype as it's used in tests.

@SimonSchick
Copy link
Contributor Author

boop

@SimonSchick
Copy link
Contributor Author

Hey @sushantdhiman since you are around, I'd love to get some feedback on this :)

@SimonSchick SimonSchick force-pushed the chore/datatypes-to-classes branch 2 times, most recently from 17df1e9 to 1b94b42 Compare March 8, 2019 11:40
Copy link
Member

@janmeier janmeier left a comment

Choose a reason for hiding this comment

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

Looks like very solid work to me - There seems to be a small problem with arrays in where queries though
image

return true;
};

// TODO: Create intermediate class
Copy link
Member

Choose a reason for hiding this comment

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

👍

@SimonSchick SimonSchick force-pushed the chore/datatypes-to-classes branch 2 times, most recently from 2f1675a to 2b401f4 Compare March 8, 2019 14:57
@SimonSchick
Copy link
Contributor Author

@janmeier Not going to do the ARRAY and number base class thing for now as that's a rabbit hole I am not willing to dive into any further at this moment.

If you are interested in poking it see my branch chore/array-to-class in my fork.

Please approve this PR as is.

lib/utils.js Outdated
@@ -11,6 +11,8 @@ const operatorsArray = _.values(operators);

let inflection = require('inflection');

exports.implicitNew = require('./utils/implicitNew');
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a method classToInvokable in these utils, which is same as implicitNew. Either use that old method or get rid of classToInvokable so we only use one common method

@sushantdhiman
Copy link
Contributor

There are some unrelated changes in this PR, please rebase with master

@sushantdhiman sushantdhiman merged commit 3cd3891 into sequelize:master Mar 9, 2019
@SimonSchick SimonSchick deleted the chore/datatypes-to-classes branch March 9, 2019 11:04
sergey-kudriavtsev added a commit to sergey-kudriavtsev/sequelize that referenced this pull request Sep 21, 2019
1) The design does not make sense, all parameters are constants and are independent of the execution parameters.
2) lack of support for packers (webpack, browserify)
3) unusable design in es6
https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-dynamic-require.md
sergey-kudriavtsev added a commit to sergey-kudriavtsev/sequelize that referenced this pull request Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants