-
Notifications
You must be signed in to change notification settings - Fork 591
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
feat: use a common tsconfig for the monorepo #1297
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1297 +/- ##
===========================================
+ Coverage 73.04% 89.70% +16.66%
===========================================
Files 287 151 -136
Lines 12730 3129 -9601
Branches 2922 576 -2346
===========================================
- Hits 9299 2807 -6492
+ Misses 3431 322 -3109
Continue to review full report at Codecov.
|
@trivikr @AllanFly120 since this is my first major commit and it could cause some problems if I missed something, can you checkout the branch and make sure everything works and is solid on your local? |
Updated client-* Co-authored-by: Trivikram Kamat <[email protected]>
This PR needs rebase to include |
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.
One minor comment for removing build and build-es.
Also, it looks like karma-credential-loader is not rebased from master:
- https://github.com/aws/aws-sdk-js-v3/tree/aforsyth/root-tsconfig/packages
- https://github.com/aws/aws-sdk-js-v3/tree/master/packages
The changes would be required in karma-credential-loader too.
All done. My rebase screwed up so ended up just doing it in a merge. karma-credential-loader has changes propagated |
Co-authored-by: Trivikram Kamat <[email protected]>
/** | ||
* Nice to have: | ||
*/ | ||
// "preserveConstEnums": true, |
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'm fine if you uncomment it.😊
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.
We can't uncomment without massive changes across the repo. Lets address this in a followup
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue #, if available:
#1026
Internal: JS-1866
Description of changes:
tsconfig.json
and roottsconfig.cjs.json
andtsconfig.es.json
.tsconfig.cjs.json
and roottsconfig.es.json
in their local version. Those are themselves built from the historicaltsconfig.json + tsconfig.test.json
. The previouspackages/*/tsconfig.json
andpackages/*/tsconfig.test.json
have been removed../build
and one esnext (es) in./build-es
. Packages have also been updated to provide abuild:es
and abuild:cjs
command.FAQ:
"noUnusedParameters": true,
everywhere but to keep this PR reasonable i've chosen not to include them here.Experimental branch reference:
#1279
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.