-
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
Changes from 5 commits
e670319
12bb08a
b39b9c0
b509817
5a22a03
3d05f35
4c5bfeb
b53269a
4043fec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
--- | ||
"@aws-amplify/ui-react-core": patch | ||
"@aws-amplify/ui-react-native": patch | ||
"@aws-amplify/ui-react": patch | ||
"@aws-amplify/ui": patch | ||
--- | ||
|
||
chore(tsconfig): add configs directory and ts configuration |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
# Overview | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This readme is awesome! |
||
|
||
Base Configuration files for platform (example `@aws-amplify/ui-vue`) and utility packages (example `@aws-amplify/ui-react-core`) in the Amplify UI monorepo. | ||
|
||
## Package Types | ||
|
||
There are currently 3 types of packages that Amplify UI supports: | ||
|
||
- **Utility Packages:** Packages shared across all platforms | ||
|
||
- `@aws-amplify/ui` | ||
|
||
- **React Utility Packages:** Non-UI React utilites and hooks shared between React and React Native platform packages | ||
|
||
- `@aws-amplify/ui-react-core` | ||
|
||
- **Platform UI Packages:** | ||
- `@aws-amplify/ui-angular` | ||
- `@aws-amplify/ui-react` | ||
- `@aws-amplify/ui-react-native` | ||
- `@aws-amplify/ui-vue` | ||
|
||
## Typescript | ||
|
||
To prevent drift between the `compilerOptions` of the _tsconfig.json_ files in the Amplify UI monorepo as more packages are added, base Typescript `compilerOptions` configurations are stored within the [ts](./ts) directory. The **[Base Config](./ts/base.json)** is extended directly or indirectly in all platform and utility packages. | ||
|
||
- Utility package _tsconfig.json_ files directly use the **[Base Config](./ts/base.json)** | ||
- React UI/React Utility package _tsconfig.json_ files use the **[React Base Config](./ts/react.json)**, extends from the **Base Config** | ||
- React Native UI package _tsconfig.json_ files use the **[React Native Base Config](./ts/react-native.json)**, extends from the **Base Config** | ||
|
||
A basic visualization of the extension hierarchy: | ||
|
||
```tree | ||
base.json | ||
├── react.json | ||
└── react-native.json | ||
``` | ||
|
||
### Strict Mode | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about naming this to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Edit: also feel like "prod" is more aligned to application stages ¯\(ツ)/¯ |
||
|
||
To allow type support and linting in _\_\_tests\_\__ and _\_\_mocks\_\__ directories during development, package level _tsconfig.json_ include all _.ts_ and _.tsx_ files. For builds, package level _tsconfig.dist.json_ files extend from their sibling _tsconfig.json_ file overriding the `excludes` value with the _\_\_tests\_\__ and _\_\_mocks\_\__ directories. | ||
|
||
**Example:** | ||
|
||
_react-core/tsconfig.json_ | ||
|
||
```json | ||
{ | ||
"extends": "../configs/ts/react.json", | ||
"include": ["src/**/*.ts", "src/**/*.tsx"], | ||
"exclude": ["node_modules"] | ||
} | ||
``` | ||
|
||
_react-core/tsconfig.dist.json_ | ||
|
||
```json | ||
{ | ||
"extends": "./tsconfig.json", | ||
"exclude": ["node_modules", "**/__mocks__", "**/__tests__"] | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"compilerOptions": { | ||
"allowSyntheticDefaultImports": true, | ||
"baseUrl": ".", | ||
"declaration": true, | ||
"esModuleInterop": true, | ||
"importHelpers": true, | ||
"module": "esnext", | ||
"moduleResolution": "node", | ||
"strict": true, | ||
"target": "es6" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"extends": "./base.json", | ||
"compilerOptions": { | ||
"jsx": "react-native", | ||
"skipLibCheck": true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. React Native only requires There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @zhamujun Can you extend There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"target": "esnext" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"extends": "./base.json", | ||
"compilerOptions": { | ||
"jsx": "react" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Removing
|
||
"@types/react-test-renderer": "^17.0.1", | ||
"@typescript-eslint/eslint-plugin": "^5.6.0", | ||
"@typescript-eslint/parser": "^5.6.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,42 @@ | ||
// rollup.config.js | ||
import { defineConfig } from 'rollup'; | ||
import typescript from '@rollup/plugin-typescript'; | ||
import externals from 'rollup-plugin-node-externals'; | ||
|
||
// common config settings | ||
const input = ['src/index.ts']; | ||
const sourceMap = false; | ||
const tsconfig = 'tsconfig.dist.json'; | ||
|
||
const esmOutputDir = 'dist/esm'; | ||
|
||
const config = defineConfig([ | ||
// CJS config | ||
{ | ||
input: ['src/index.ts'], | ||
output: { | ||
dir: 'dist', | ||
format: 'cjs', | ||
sourcemap: false, | ||
}, | ||
input, | ||
output: { dir: 'dist', format: 'cjs' }, | ||
plugins: [ | ||
externals({ include: /^@aws-amplify/ }), | ||
typescript({ declarationDir: 'dist/types', sourceMap: false }), | ||
typescript({ declarationDir: 'dist/types', sourceMap, tsconfig }), | ||
], | ||
}, | ||
// ESM config | ||
{ | ||
input: ['src/index.ts'], | ||
input, | ||
output: { | ||
dir: 'dist/esm', | ||
dir: esmOutputDir, | ||
format: 'es', | ||
entryFileNames: '[name].mjs', | ||
preserveModules: true, | ||
preserveModulesRoot: 'src', | ||
sourcemap: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the |
||
}, | ||
plugins: [ | ||
externals({ include: /^@aws-amplify/ }), | ||
typescript({ outDir: 'dist/esm', declaration: false, sourceMap: false }), | ||
typescript({ | ||
outDir: esmOutputDir, | ||
declaration: false, | ||
sourceMap, | ||
tsconfig, | ||
}), | ||
], | ||
}, | ||
]); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"extends": "./tsconfig.json", | ||
"exclude": ["node_modules", "**/__mocks__", "**/__tests__"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,5 @@ | ||
{ | ||
"compilerOptions": { | ||
"allowSyntheticDefaultImports": true, | ||
"baseUrl": ".", | ||
"declaration": true, | ||
"esModuleInterop": true, | ||
"importHelpers": true, | ||
"jsx": "react", | ||
"module": "esnext", | ||
"moduleResolution": "node", | ||
"noImplicitAny": true, | ||
"skipLibCheck": true, | ||
"strict": true, | ||
"target": "es6" | ||
}, | ||
"extends": "../configs/ts/react.json", | ||
"include": ["src/**/*.ts", "src/**/*.tsx"], | ||
"exclude": ["node_modules"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,39 +1,46 @@ | ||
// rollup.config.js | ||
import { defineConfig } from 'rollup'; | ||
import typescript from '@rollup/plugin-typescript'; | ||
import { terser } from 'rollup-plugin-terser'; | ||
import styles from 'rollup-plugin-styles'; | ||
import externals from 'rollup-plugin-node-externals'; | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. nit: what do you think if we have a |
||
|
||
const config = defineConfig([ | ||
// CJS config | ||
{ | ||
input: ['src/index.tsx', 'src/internal.tsx'], | ||
input, | ||
output: { | ||
dir: 'dist', | ||
format: 'cjs', | ||
sourcemap: false, | ||
}, | ||
plugins: [ | ||
externals({ include: /^@aws-amplify/ }), | ||
typescript({ declarationDir: 'dist/types', sourceMap: false }), | ||
typescript({ declarationDir: 'dist/types', sourceMap, tsconfig }), | ||
terser(), | ||
], | ||
}, | ||
// ESM config | ||
{ | ||
input: ['src/index.tsx', 'src/internal.tsx'], | ||
input, | ||
output: { | ||
dir: 'dist/esm', | ||
format: 'es', | ||
entryFileNames: '[name].mjs', | ||
preserveModules: true, | ||
preserveModulesRoot: 'src', | ||
sourcemap: false, | ||
}, | ||
plugins: [ | ||
externals({ include: /^@aws-amplify/ }), | ||
typescript({ outDir: 'dist/esm', declaration: false, sourceMap: false }), | ||
typescript({ | ||
outDir: 'dist/esm', | ||
declaration: false, | ||
sourceMap, | ||
tsconfig, | ||
}), | ||
terser(), | ||
], | ||
}, | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"extends": "./tsconfig.json", | ||
"exclude": ["node_modules", "**/__mocks__", "**/__tests__"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,10 @@ | ||
{ | ||
"extends": "../configs/ts/react.json", | ||
"compilerOptions": { | ||
"allowSyntheticDefaultImports": true, | ||
"baseUrl": ".", | ||
"declaration": true, | ||
"esModuleInterop": true, | ||
"importHelpers": true, | ||
"jsx": "react", | ||
"module": "esnext", | ||
"moduleResolution": "node", | ||
"rootDir": "./src", | ||
"noImplicitAny": false, | ||
"skipLibCheck": true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added $ 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. |
||
"target": "es6" | ||
"strict": false | ||
}, | ||
"exclude": ["node_modules", "src/**/*.test.ts", "src/**/*.test.tsx"], | ||
"include": ["src/**/*.ts", "src/**/*.tsx"] | ||
"include": ["src/**/*.ts", "src/**/*.tsx"], | ||
"exclude": ["node_modules"] | ||
calebpollman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
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
causedreact-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.