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

[WIP] chore: convert to TS #235

Closed

Conversation

JoviDeCroock
Copy link

@JoviDeCroock JoviDeCroock commented Jan 25, 2019

So as suggested by @benjamn I started on the conversion to TS, this has proven to be non-trivial, so I'm already putting it out there so anyone passing by can see and give their two cents about it

#234 (comment) --> ref to comment about conversion to TS

@JoviDeCroock
Copy link
Author

Hmm, the tests seem like something else... This feels really weird since we're working with a .default AND a module.exports. The default import is working but the other isn't... Some parts of this code feel really weird

@JoviDeCroock
Copy link
Author

JoviDeCroock commented Jan 27, 2019

In the tests two issues are still open

V0.11: This test is not being executed in master atm so I did not migrate it to TypeScript
Normal tests: unique fragments › ignores duplicate fragments from second-level imports when using the webpack loader, the import for that imaginary .graphql file is not working anymore. It fails on eval

The // TODO comments in src/index.ts need a review since I don't have enough knowledge about the underlying aspect myself.

Also we need to find a way to test with these different versions but then with our new jest tests.

Sorry for leaving this much open but it's quite hard to "choose" an infrastructure.

I'll probably add test to the CI when the changes are approved

@jnwng
Copy link
Contributor

jnwng commented Feb 6, 2019

V0.11: This test is not being executed in master atm so I did not migrate it to TypeScript

tests for graphql@0.11 get executed via test-all-versions (see here)

overall, i think translating this library to typescript is a useful effort. however. i would want to ensure that tests still pass — we don't want to drop any support unnecessarily, plus we'd expect the same behavior here.

additionally, i don't feel comfortable merging the parts that are "unknown" in TypeScript-land, so it'd be valuable to have someone review this who might have the answers we need.

in the pursuit of moving this forward, can we break this down into smaller pieces to get pieces of it through, first? perhaps splitting this into 1) infrastructural changes (e.g., the typescript toolchain) 2) typescript interfaces and 3) tests? that might allow us to have a more focused look at the individual parts of the library that are being ported.

thoughts?

@JoviDeCroock
Copy link
Author

JoviDeCroock commented Feb 6, 2019

@jnwng
I'm up to split the PR up into multiple parts, I'll start off by making an infrastructure PR conforming to apollo-client. Then make multiple others that will be targetted at the infrastructure PR, does this sound okay for you?

Steps:

  1. add TS infrastructure
  2. convert the source to .ts
  3. update way of building
  4. retarget loader.js
  5. tests
  6. Remove obsolete dependencies

Also when I look at the travis CI jobs (ex: https://travis-ci.org/apollographql/graphql-tag/jobs/476452576) I see that 0.11 fails, that's why I thought that.

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.

2 participants