-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Prepare for concurrently developing in TS and Flow #2139
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
Conversation
| "private": true, | ||
| "main": "index", | ||
| "module": "index.mjs", | ||
| "types": "index.d.ts", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| @@ -1,4 +1,4 @@ | |||
| import { syntaxError } from '../error'; | |||
There was a problem hiding this comment.
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:
graphql-js/src/language/lexer.js
Line 5 in 85d4c99
| import { syntaxError } from '../error/syntaxError'; |
Also, we never use index files inside the library only direct imports from files.
There was a problem hiding this comment.
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
|
@JacksonKearl Looks great 👍 After we stabilize Also, I need to be able to rebase |
|
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 |
|
@JacksonKearl After I merge your |
|
@JacksonKearl Also can you please try that code coverage is working with TS files? |
|
@JacksonKearl Would be great if can try all NPM |
|
@IvanGoncharov Added support for code coverage checking in TS files |
|
Do we have a 14.5.4 cut yet? |
|
Will look into the scripts today |
|
Got ES lint to lint the typescript files, and using the recommended rules from https://github.com/typescript-eslint/typescript-eslint |
Sorry for the delay, published |
1023db7 to
8a7955d
Compare
- 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
83495da to
43b7645
Compare
.d.tsfiles from./tstypesinto./src./for both building and type checking the projectresources/build.jsto work with.tsand.d.tsfiles./package.jsonjsutils/dedent.jstojsutils/dedent.tsandjsutils/dedent.js.flowas an example of conversion proccessjsutils/__tests__/dedent-test.jstojsutils/__tests__/dedent-test.tsas an example of conversion proccess