-
Notifications
You must be signed in to change notification settings - Fork 314
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
chore(tsconfig): add configs directory and ts configuration #3497
Conversation
🦋 Changeset detectedLatest commit: 4043fec The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
format: 'es', | ||
entryFileNames: '[name].mjs', | ||
preserveModules: true, | ||
preserveModulesRoot: 'src', | ||
sourcemap: false, |
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.
Removed the output.sourcemap
option in all of the ESM and CJS rollup configs as they are extraneous. The setting of sourceMap
to false
in the typescript
plugin options handles it
"extends": "./base.json", | ||
"compilerOptions": { | ||
"jsx": "react-native", | ||
"skipLibCheck": 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.
React Native only requires skipLibCheck
due its incompatible typing with @types/node
, see DefinitelyTyped/DefinitelyTyped#15960 for further context
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.
Too bad json doesn't support comments. But maybe we can include this info in the readme?
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.
What if we convert to .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.
@zhamujun Can you extend .ts
files in a tsconfig?
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.
Found a discussion on it microsoft/TypeScript#25271
And there's a package to do it https://www.npmjs.com/package/tsconfig.js
Maybe not worth our time to convert and introduce another dependency
|
||
All packages are in TS strict mode by default unless opted out at the package configuration level. | ||
|
||
### Usage of _tsconfig.dist.json_ |
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.
What do you think about naming this to _tsconfig.prod.json_
so that this is more explicit?
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.
Not feeling super strongly (so basically feeling minimally strongly) about the name, but prefer tsconfig.dist.json because:
- we already use it
- it aligns with the dist output directory
Edit: also feel like "prod" is more aligned to application stages ¯\(ツ)/¯
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.
LGTM, just one nit above 😄
@@ -0,0 +1,66 @@ | |||
# Overview |
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.
This readme is awesome!
"extends": "./base.json", | ||
"compilerOptions": { | ||
"jsx": "react-native", | ||
"skipLibCheck": 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.
Too bad json doesn't support comments. But maybe we can include this info in the readme?
// common config settings | ||
const input = ['src/index.ts', 'src/internal.ts']; | ||
const sourceMap = false; | ||
const tsconfig = 'tsconfig.dist.json'; |
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.
nit: what do you think if we have a const commonConfigSettings = {...}
object? Because it's hard to tell where the "common config settings" ends from the comment
"extends": "./base.json", | ||
"compilerOptions": { | ||
"jsx": "react-native", | ||
"skipLibCheck": 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.
What if we convert to .ts?
…perties definition
b509817
declare module 'style-dictionary/lib/utils/flattenProperties' { | ||
// based on the typing specified by style-dictionary: | ||
// https://github.com/amzn/style-dictionary/blob/main/lib/utils/flattenProperties.js | ||
export default function flattenProperties(properties: object, to_ret?: Array): Array; | ||
} |
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 added two SD related declaration files in containing the same function signatures in #3312 and #3400.
The definitions for the other two functions were identical, but the definitions of flattenProperties
differ. Ultimately usage of the flattenProperties
util is from the other file, where the modules decalaration files are appended with .js, making this file unused
@@ -19,8 +19,8 @@ declare module 'style-dictionary/lib/utils/deepExtend.js' { | |||
declare module 'style-dictionary/lib/utils/flattenProperties.js' { | |||
// based on the typing specified by style-dictionary: | |||
// https://github.com/amzn/style-dictionary/blob/main/lib/utils/flattenProperties.js#L16-L24 | |||
export default function flattenProperties( | |||
export default function flattenProperties<T, K = T extends Array<infer TK> ? TK : never>( |
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.
…ct native types from react-core
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.
Removing skipLibCheck
exposed some linting issues that were not caught on build runs.
docs/package.json
Outdated
@@ -28,7 +28,7 @@ | |||
"@mdx-js/react": "^2.1.0", | |||
"@moxy/next-compile-node-modules": "^2.1.1", | |||
"@xstate/inspect": "^0.4.1", | |||
"aws-amplify": "^5.0.1", | |||
"aws-amplify": "^5.0.16", |
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.
Removing skipLibCheck
caused react-core
to have linting failures from a type issue in @aws-amplify/storage@5.0.1
which was fixed in aws-amplify/amplify-js#10696.
Upgrading aws-amplify
here as it is the version that gets hoisted in to the top level node_modules.
"module": "esnext", | ||
"moduleResolution": "node", | ||
"rootDir": "./src", | ||
"noImplicitAny": false, | ||
"skipLibCheck": 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.
Added skipLibCheck
back as removing exposed a linting error caused by a type error in maplibre-gl-js-amplify:
$ yarn workspace @aws-amplify/ui-react lint
$ tsc --noEmit && eslint src --ext .js,.ts,.tsx
../../node_modules/maplibre-gl-js-amplify/lib/esm/AmplifyGeofenceControl/AmplifyMapDraw.d.ts:1:23 - error TS2688: Cannot find type definition file for 'mapbox__mapbox-gl-draw'.
1 /// <reference types="mapbox__mapbox-gl-draw" />
~~~~~~~~~~~~~~~~~~~~~~
Found 1 error in ../../node_modules/maplibre-gl-js-amplify/lib/esm/AmplifyGeofenceControl/AmplifyMapDraw.d.ts:1
Will add a github issue there to address it and can remove the flag once it's fixed.
@@ -44,7 +44,6 @@ | |||
"@types/jest": "^26.0.23", | |||
"@types/react": "^17.0.2", | |||
"@types/react-dom": "^17.0.13", | |||
"@types/react-native": "^0.63.45", |
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.
Removing skipLibCheck
in react-core
exposed the incompatibility issue with @types/react-native
and @types/node
.
@types/react-native
is unused in this package, and most likely will not be required in the future. If it is can turn back on skipLibCheck
at that point
…les aws-amplify versions
@@ -28,7 +28,7 @@ | |||
"@mdx-js/react": "^2.1.0", | |||
"@moxy/next-compile-node-modules": "^2.1.1", | |||
"@xstate/inspect": "^0.4.1", | |||
"aws-amplify": "^5.0.1", | |||
"aws-amplify": "latest", |
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.
To ensure we don't have weird aws-amplify
install behavior, aligned the dep version here to what our example apps use, which is latest
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.
Related: #3497 (comment)
@@ -14,7 +14,7 @@ | |||
"directory": "packages/angular/projects/ui-angular" | |||
}, | |||
"peerDependencies": { | |||
"aws-amplify": "5.x.x" | |||
"aws-amplify": ">= 5.0.1" |
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.
Aligned the peerDep version here with other platform packages. 5.x.x
was the incorrect version here, aws-amplify@5.0.0
had type issues that were fixed in 5.0.1
(same comment for ui-react
)
"extends": "./base.json", | ||
"compilerOptions": { | ||
"jsx": "react-native", | ||
"skipLibCheck": 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.
Found a discussion on it microsoft/TypeScript#25271
And there's a package to do it https://www.npmjs.com/package/tsconfig.js
Maybe not worth our time to convert and introduce another dependency
Description of changes
Add configs directory and shared Typscript configuration handling for
ui
,ui-react
,ui-react-core
andui-react-native
packages:compilerOptions
config filesrollup
config filesui-react
and update .eslintrc.js accordinglyIssue #, if available
Description of how you validated changes
Checklist
yarn test
passessideEffects
field updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.