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

Prepare for concurrently developing in TS and Flow #2139

Closed
wants to merge 13 commits into from

Conversation

JacksonKearl
Copy link

  • Move .d.ts files from ./tstypes into ./src
  • Add tsconfigs to ./ for both building and type checking the project
  • Modify resources/build.js to work with .ts and .d.ts files
  • Add TS build scripts to ./package.json
  • Convert jsutils/dedent.js to jsutils/dedent.ts and jsutils/dedent.js.flow as an example of conversion proccess
  • Convert jsutils/__tests__/dedent-test.js to jsutils/__tests__/dedent-test.ts as an example of conversion proccess

.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -6,6 +6,7 @@
"private": true,
"main": "index",
"module": "index.mjs",
"types": "index.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Since we will ship .d.ts alongside of .js files, why do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

TS docs say its good practice:

Also note that if your main declaration file is named index.d.ts and lives at the root of the package (next to index.js) you do not need to mark the "types" property, though it is advisable to do so.

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@JacksonKearl Thanks for link now it make sense.
Can you please create PR against master?

package.json Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
import { syntaxError } from '../error';
Copy link
Member

Choose a reason for hiding this comment

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

Please extract all changes in .d.ts into PR against master.
Please keep d.ts files in sync with flow files:

import { syntaxError } from '../error/syntaxError';

Also, we never use index files inside the library only direct imports from files.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, up at #2140

@IvanGoncharov
Copy link
Member

@JacksonKearl Looks great 👍
Also I need to look into tsc compiler before I can review build.js part and tsconfig files.

After we stabilize 14.5.x I want to release 15.0.0-alpha.1 with all expected breaking changes (but without TS conversion) so Apollo, Facebook, GraphiQL, Nexus, typed-graphql, and other big API clients can test it locally and give early feedback on proposed breaking changes. Plus we would have it as baseline quickly check if the particular issue was added during TS conversion or existed before.

Also, I need to be able to rebase 15.x.x on top of master until we fully stabilize 14.5.x a line.
So if everything goes smooth we can merge it around Monday.

@JacksonKearl
Copy link
Author

Alright, I'll be out of a job starting Friday so definitely let me know if there's anything I can do before then. I'll put up a PR against master with the .d.ts and package lock changes today

@IvanGoncharov
Copy link
Member

@JacksonKearl After I merge your .d.ts changes into master I will release 14.5.4 and would be extremely useful if you migrate as many Apollo projects as possible to use this version (or at least try it locally).

@IvanGoncharov
Copy link
Member

@JacksonKearl Also can you please try that code coverage is working with TS files?

@IvanGoncharov
Copy link
Member

@JacksonKearl Would be great if can try all NPM scripts and ensure that they are working with TS files. For example, will ESLint validate TS files?
Also can we get TS-specific rules from https://github.com/typescript-eslint/typescript-eslint

@JacksonKearl
Copy link
Author

@IvanGoncharov Added support for code coverage checking in TS files

@JacksonKearl
Copy link
Author

Do we have a 14.5.4 cut yet?

@JacksonKearl
Copy link
Author

Will look into the scripts today

@JacksonKearl
Copy link
Author

Got ES lint to lint the typescript files, and using the recommended rules from https://github.com/typescript-eslint/typescript-eslint

@IvanGoncharov
Copy link
Member

Do we have a 14.5.4 cut yet?

Sorry for the delay, published 14.5.4 📦

- Move `.d.ts` files from `./tstypes` into `./src`
- Add tsconfigs to `./` for both building and type checking the project
- Modify `resources/build.js` to work with `.ts` and `.d.ts` files
- Add TS build scripts to `./package.json`
- Convert `jsutils/dedent.js` to `jsutils/dedent.ts` and `jsutils/dedent.js.flow` as an example of conversion proccess
- Convert `jsutils/__tests__/dedent-test.js` to `jsutils/__tests__/dedent-test.ts` as an example of conversion proccess

# Conflicts:
#	src/execution/execute.d.ts
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

2 participants