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

TypeScript #2698

Closed
wants to merge 22 commits into from
Closed

TypeScript #2698

wants to merge 22 commits into from

Conversation

tgriesser
Copy link
Member

I think I mentioned this before, but this is the beginning of an experimental migration toward TypeScript. This may not get merged, so just consider this an experiment/WIP for now.

For starters, we'll have the TypeScript compiler building instead of Babel and just compile the js files, so we can migrate files one-at-a-time, but eventually some of the goals for this is:

  • Full (strict) type coverage for all files, converted from js -> ts
  • No export default anywhere, making it simpler to use an editor to do full project refactors based on the symbol
  • Strong typings for splitting dialects out from the core
  • Good use of generics to be able to return types (and possibly column names) in the builder
  • Jest instead of mocha/tape - leveraging jest features like snapshot tests
  • Simplified tests, a lot of things feel like a hack right now
  • Less integration tests - shouldn't need to re-test things that are tested in the various dialect modules, we should assume that if the types check it's good, and we run a few simple queries just to be sure
  • Stronger guarantees when doing more ambitious things like moving to async/await
  • An overall better foundation for adding more ambitious features to the core

Probably will be broken for a bit, will be welcoming contributions once some of the core config for this is ironed out and we have some examples of the conventions to follow.

@tgriesser
Copy link
Member Author

Just incase anyone is wondering, I'm going to be offline for the next 2-3 weeks but will pick this up when I get back.

@JakeGinnivan
Copy link
Contributor

JakeGinnivan commented Aug 21, 2018

@tgriesser if you need a hand with this let me know :)

@tgriesser
Copy link
Member Author

@JakeGinnivan thanks! Kind of gave up on this approach, going with more of a full scale internal rewrite so we can actually properly type all the internals from the start, and at the same time simplify/strip out a lot of unneeded layers (formatter, compiler, etc.).

It's coming along, I'm going to work to get it working in one of the dialects (probably mysql) and then I'll open a PR... at which point I'll definitely be reaching out for feedback/help migrating the other dialects.

@JakeGinnivan
Copy link
Contributor

@tgriesser another option is to upgrade to babel 7 (which can parse the TypeScript types and ignore them), then setup tsc with --allow-js. Then you can start incrementally adding types and getting the benefit now.

@elhigu
Copy link
Member

elhigu commented Sep 28, 2018

@JakeGinnivan we just updated to babel 7 two days ago :)

@tgriesser
Copy link
Member Author

Closing in favor of https://github.com/tgriesser/knex-next (still super early on, nothing is actually setup from a connection standpoint, so don't look too much into it or expect it to work)

@tgriesser tgriesser closed this Oct 5, 2018
@tgriesser tgriesser deleted the ts branch October 5, 2018 15:55
@tgriesser
Copy link
Member Author

@JakeGinnivan I'd personally rather have no types internally than partial/loose types. Without --strict and usage across the whole codebase I fail to see any benefit over using the definitions from Definitely Typed from an end user perspective.

Also there's a ton of how the library is structured more generally I'd like to address with this effort and I believe TypeScript and some of the new ideas in the repo linked above will provide a path forward on that.

@wubzz
Copy link
Member

wubzz commented Oct 5, 2018

@tgriesser knex-next looks interesting, big step up. Would like to help when possible. I guess this early on would just be getting in the way, but just FYI.

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

4 participants