-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
package.json
Outdated
@@ -6,6 +6,7 @@ | |||
"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.ts
files from./tstypes
into./src
./
for both building and type checking the projectresources/build.js
to work with.ts
and.d.ts
files./package.json
jsutils/dedent.js
tojsutils/dedent.ts
andjsutils/dedent.js.flow
as an example of conversion proccessjsutils/__tests__/dedent-test.js
tojsutils/__tests__/dedent-test.ts
as an example of conversion proccess