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

feat: add typescript typings #10287

Merged
merged 3 commits into from Jan 18, 2019

Conversation

7 participants
@SimonSchick
Copy link
Contributor

SimonSchick commented Dec 19, 2018

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

Closes #9285

TODO: Documentation?

Please note that these typings are based on https://github.com/types/sequelize but have been heavily modified and updated for v5 use by me.

These typings are generally not compatible with the current DT typings as they are considered incomplete and outdated (no class support etc).

I'd ask like to ask types/sequelize contributors:
@felixfbecker
@louy
@eseliger
@ppetzold
@prokopsimek
@lumaxis
@C45tr0
@d6u
@modulitos
@rokoroku
@MorpheusXAUT
@SwadicalRag
@JD-Robbs
@munsellj
@Thylossus

And DT contributors:
@samuelneff
@CodeAnimal
@drinchev
@babolivier
@kukoo1
@oktapodia
@MorpheusXAUT
@TitaneBoy
@zjy01
@nidzov
@Raigen
@todd
@nrschultz
@Thomas-B
@Antoine38660
@smff

to leave feedback on this PR

@felixfbecker @louy as you are core authors of types/sequelize, do you have any concerns regarding the licensing?
Currently your package.json only specifies ISC without a license file which is generally compatible with the MIT license of sequelize.

@SimonSchick SimonSchick force-pushed the SimonSchick:feat/ts branch from 7ab256b to 103f8b0 Dec 19, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #10287 into master will decrease coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10287      +/-   ##
==========================================
- Coverage   96.27%   96.27%   -0.01%     
==========================================
  Files          68       68              
  Lines        9772     9763       -9     
==========================================
- Hits         9408     9399       -9     
  Misses        364      364
Impacted Files Coverage Δ
lib/hooks.js 97.01% <ø> (-0.05%) ⬇️
lib/sequelize.js 96.35% <100%> (-0.02%) ⬇️
lib/utils.js 98.23% <50%> (-0.05%) ⬇️

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 9c0f041...5acca0e. Read the comment docs.

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Jan 3, 2019

Is this ready or still under WIP @SimonSchick ?

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Jan 3, 2019

@sushantdhiman I was hoping for some feedback, I highly doubt that this PR is ready as is.
Unfortunately none of the people I mentioned replied.

I'm not exactly sure what to add in terms of documentation.
In particular the licensing still needs to be figured out.

@drinchev

This comment has been minimized.

Copy link

drinchev commented Jan 3, 2019

Unfortunately I'm not using sequelize at the current project I'm working on and the last one is with outdated typescript version. Saying this it is really difficult for me to test the PR.

Anyway the definition file for sequelize is somehow a different program ( e.g. BelongsToGetAssociationMixin are not part of sequelize, but just a helper to make your code easier to read ), so it is as well opinionated and not 1:1 to the code base. For me this always meant that there is a need to have additional docs on the project website how to use the typings.

Thanks guys for adding the definition files to the project. 👍

@todd

This comment has been minimized.

Copy link

todd commented Jan 3, 2019

@SimonSchick I can try to find some time to put this PR through its paces, but I doubt it'll be comprehensive. It may be worth creating a test file like the one we use in the DT repo to validate that the typings conform to the API as you believe they should. I can help get the ball rolling on that if you're interested.

@todd

This comment has been minimized.

Copy link

todd commented Jan 3, 2019

Link to the DT test file: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/sequelize/sequelize-tests.ts

Moment did something similar after an errant typo broke their declaration files:
Discussion: moment/moment#3280
Work: moment/moment#3282

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Jan 3, 2019

@todd I already added test files?

@todd

This comment has been minimized.

Copy link

todd commented Jan 3, 2019

@SimonSchick Oh, yikes - I clearly didn't do a deep dive on the changes in this PR 😆

If the tests pass, I assume that's probably good enough to get started, at least for pre-5.0. But, again, I'll try to put the code in this PR through its paces in the next couple of days and see if I run across anything.

From a purely pragmatic standpoint, I might recommend contributing these changes to Types or DT for 5.0 before adding them directly to this package. Not sure if we're worried about churn, but the DT declaration files see somewhat consistent changes and updates (maybe ~2-3 PRs/month on average). Totally up to you guys, though.

@C45tr0

This comment has been minimized.

Copy link

C45tr0 commented Jan 3, 2019

I would love to have these definitions inside of the primary repo. I find they stay in sync easier that way. Thanks for starting this!

@SimonSchick Was wondering why you removed the Utils.sliceArgs inside of this pr and not a separate PR?

Also, I'll try and take a look at these this weekend also. Looking to use v5 in my next project anyway.

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Jan 4, 2019

sliceArgs slipped into this PR, if that's an issue I will revert that.

@SimonSchick SimonSchick changed the title WIP: feat: add typescript typings feat: add typescript typings Jan 15, 2019

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Jan 15, 2019

Removed WIP tag for now, I have not really gotten any feedback but I've been using these typings in a few projects and they seem pretty stable.

});

sequelize
.query('INSERT into test set test=1', {

This comment has been minimized.

@rokoroku

rokoroku Jan 15, 2019

Contributor

the indentation looks ugly here 😉

This comment has been minimized.

@SimonSchick

SimonSchick Jan 15, 2019

Author Contributor

I'd add linting for TS but I don't want to bloat up the CI.

@rokoroku

This comment has been minimized.

Copy link
Contributor

rokoroku commented Jan 15, 2019

We can improve typings after it ships to the npm prerelease or release, integrating with real projects.

@sushantdhiman
Copy link
Member

sushantdhiman left a comment

Going to merge this @SimonSchick , once these get deployed community can improve them.

@sushantdhiman sushantdhiman merged commit 26547bc into sequelize:master Jan 18, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Jan 18, 2019

@SimonSchick Great work 💯

@SimonSchick SimonSchick deleted the SimonSchick:feat/ts branch Jan 18, 2019

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Jan 18, 2019

Oh boy.

@janmeier

This comment has been minimized.

Copy link
Member

janmeier commented Jan 18, 2019

I wonder if we can somehow generate the api docs from typescript typings, or typescript typings from jsdoc? It seems kinda of stupid to keep two different forms of api docs in the code

@SimonSchick

This comment has been minimized.

Copy link
Contributor Author

SimonSchick commented Jan 18, 2019

I've seen this happen with GoogleChrome/puppeteer#3744 but I strongly suggest against it, not only will it add a build step but will also be much less flexible than raw typings.

Generating the docs from typing files seems like a much better approach

Ideally we would just move sequelize to TS entirely, this would give all benefits.

I believe the counter arguments were:

  • More complicated
    • It actually makes the code easier to understand without ctrl+f every function call
  • Build step (no github install)
    • Invalid imo, this is what pre-releases or nightly builds are for

But obviously that's a stupid amount of work :)

@janmeier

This comment has been minimized.

Copy link
Member

janmeier commented Jan 18, 2019

Generating the docs from typing files seems like a much better approach

Yeah that sounds more reasonable to me as well - Do you have any examples of projects doing it this way?

Ideally we would just move sequelize to TS entirely, this would give all benefits.

I don't have any specific count arguments to this, other than the amount of work required

I see that all they typings are now stored in separate files - Is there no way to store them "closer" to the code - I feel like it would be easier to make sure that typings are kept in check with the actual code if they are located closely together, so that when you add a new argument to a method, you remember to add it to the typings as well. Perhaps we should add "update typings" to the PR checklist @sushantdhiman ?

@janmeier

This comment has been minimized.

Copy link
Member

janmeier commented Jan 18, 2019

Also, great work! :) 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.