-
Notifications
You must be signed in to change notification settings - Fork 2
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
FEI-4957.3: Add tsconfig.json and library type definitions #524
Conversation
🦋 Changeset detectedLatest commit: 055ad4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Size Change: 0 B Total Size: 4.36 kB ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 59 59
Lines 840 840
Branches 231 224 -7
=========================================
Hits 840 840
Continue to review full report at Codecov.
|
@@ -59,6 +64,7 @@ | |||
"rollup-plugin-filesize": "^9.1.2", | |||
"rollup-plugin-preserve-shebangs": "^0.2.0", | |||
"rollup-plugin-terser": "^7.0.2", | |||
"typescript": "^4.9.5", |
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.
wooohoo!
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.
Some questions inline that I think need addressing (either with a change or at least a response to clarify things). Otherwise, it's exciting progress! 🎉
@@ -71,6 +77,7 @@ | |||
"clean": "rm -rf packages/wonder-stuff-*/dist && rm -rf packages/wonder-stuff-*/node_modules", | |||
"coverage": "yarn run jest --coverage", | |||
"flow:ci": "flow check --max-workers 0", | |||
"format": "prettier --write .", |
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.
question: Why was this needed? Does this mean eslint will no longer apply prettier formatting in its autofixes?
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.
I forgot that I could've used eslint --fix
to the formatting. 🙃
"compilerOptions": { | ||
/* Visit https://aka.ms/tsconfig to read more about this file */ | ||
|
||
/* Projects */ |
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.
question: Long term, do we need all these commented out things? From the perspective of a future maintainer, it's not clear to me if these are temporarily omitted things that used to be there or just helpful documentation - especially since they have values associated to them. Should we delete the commented out bits now and rely on the doc link for more info?
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.
Probably not. I ended up removing these in #528. I'll re-add the link to https://aka.ms/tsconfig in that PR.
b1c43c8
to
2bdfca9
Compare
acc55bd
to
de6491e
Compare
de6491e
to
055ad4a
Compare
Summary:
Before we can run the script to convert Flow to TypeScript, we need to have a tsconfig.json in place along with type definitions for as many of the third-party libraries that we're using as possible. The next PR will contain the migrated files.
Issue: FEI-4957
Test plan: