build: upgrade ts to 3.8 to use export type syntax #9122

hkjpotato · 2021-10-28T02:57:49Z

Description of changes

Upgrade typescript from 3.7 to 3.8, so as to facilitate the upcoming esm bundle by esbuild, BUT it is backward compatible to current version using version selection. Its backward compatibility has been tested by running through all integ tests, it works in IDE like vscode as well.

Before TS3.8, "type" and "value" can be exported together implicitly.

export interface A // type only, dropped in runtime
export enum B // a runtime value

import { A, B} from ... // isolatedModules tsc gets confused

The mixing of type and value creates confusion for both human and machine, especially for build tool like ESBuild/Vite, which relies on TS compiler to work in "isolatedModules" mode.

After TS3.8, we can define the import/export more explicitly.

export interface A 
export enum B 

import type { A } from ...
import { B } from ...

See detailed from typescript announcement https://devblogs.microsoft.com/typescript/announcing-typescript-3-8/#type-only-imports-exports, See similar issue related to esbuild support: evanw/esbuild#1398.

Related changes due to this upgrade:

use typesVersions to support backward compatibility: the only file changed is the index.d.ts for API, thus we redirects it to a separate file when customer is using a version < 3.8. This change has been tested by integ tests. The syntax follows the TS doc suggestion.
Local tsconfig of analytics category is removed to use the centralized one, similar to other package. This can help unblock the esbuild project as well.
- Its jest tsconfig is updated accordingly.
The prettier upgrade is required to accept the new TS syntax
Support TypeScript 3.8 prettier/prettier#7263
- to support our monorepo structure, the local version used by react-native is kept by "nohoist".

Please note:

This is a small steps to move to new TS version, we have other places which can be rewritten to use type import and export, but to reduce the impact, I only make change api/index.ts where esBuild cannot infer the "type".
This does not affect the other build outputs. See TS3.8 breaking change
The local useless tsconfig under analytics is removed. Our build process currently uses the centralized TS config specified in scripts/build.js.
- Except for amazon-cognito-identity-js, which still relies on babel to transpile, and will be addressed separately.

Issue #, if available

Vite team @sodatea has pointed out a related issue for reference evanw/esbuild#1525

Description of how you validated changes

This change has been verified by hosting ESM at localhost, based on @elorzafe's origin POC. The localhost modules can be used by ESM syntax directly in browser.

<script type="module" crossorigin="anonymous">

import * as core from  'http://localhost:8080/packages/core/dist/core-esm.js';
import * as auth from 'http://localhost:8080/packages/auth/dist/auth-esm.js';
import * as api from 'http://localhost:8080/packages/api/dist/api-esm.js';
import * as datastore from 'http://localhost:8080/packages/datastore/dist/datastore-esm.js';
import * as geo from 'http://localhost:8080/packages/geo/dist/geo-esm.js';
import * as cache from 'http://localhost:8080/packages/cache/dist/cache-esm.js';
import * as storage from 'http://localhost:8080/packages/storage/dist/storage-esm.js';
import * as predictions from 'http://localhost:8080/packages/predictions/dist/predictions-esm.js';
import * as interactions from 'http://localhost:8080/packages/interactions/dist/interactions-esm.js';
import * as pubsub from 'http://localhost:8080/packages/pubsub/dist/pubsub-esm.js';
import * as xr from 'http://localhost:8080/packages/xr/dist/xr-esm.js';
import * as apigraphql from 'http://localhost:8080/packages/api-graphql/dist/api-graphql-esm.js';
import * as apirest from 'http://localhost:8080/packages/api-rest/dist/api-rest-esm.js';

...

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

elorzafe · 2021-10-28T17:40:03Z

LGTM, we need to discuss with the team the impact this will have with Angular

hkjpotato · 2021-10-28T19:21:48Z

thanks @ashika01 to help on the prettier issue!!!! For context:
The aws-amplify-react-native has its own prettier version 2.4.1 which also has a devDependency eslint-pluggin-prettier. If the local prettier somehow use the same version as the global one, that plugin will not be installed in the local node_modules.

Will find a way to resolve this conflict.

related
prettier/eslint-plugin-prettier#396

wlee221 · 2021-10-28T20:21:24Z

LGTM, we need to discuss with the team the impact this will have with Angular

LGTM, Angular 9+ supports TS 3.7+.

ashika01 · 2021-10-28T21:42:42Z

nohoist solved the build issue 🎉 but looks some unit tests are failing

hkjpotato · 2021-10-28T22:58:03Z

I have sync up with the team on previous issue for angular UI support, need to do more testing on it.
The unit test issue can be fixed by updating the jest-ts config with esModuleInterop, like the other packages, yet it raises one question in my mind:
- why we use a separate ts config for unit test? shouldn't our unit test run against the build code instead of the source code?

ashika01 · 2021-10-28T23:01:17Z

why we use a separate ts config for unit test? shouldn't our unit test run against the build code instead of the source code?

It might be how we originally setup. But doesn't have to be the case, if anything we should find a way why it aint pulling the one from root level and make changes accordingly. That said, if thats a huge effort we can do it outside of this PR (as a separate item)

codecov-commenter · 2021-10-28T23:06:13Z

Codecov Report

Merging #9122 (5cc9a92) into main (fbe1f2e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #9122   +/-   ##
=======================================
  Coverage   78.07%   78.07%           
=======================================
  Files         250      250           
  Lines       18122    18122           
  Branches     3891     3891           
=======================================
  Hits        14148    14148           
  Misses       3844     3844           
  Partials      130      130

Impacted Files	Coverage Δ
...ges/analytics/src/Providers/AWSPinpointProvider.ts	`77.74% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe1f2e...5cc9a92. Read the comment docs.

hkjpotato · 2021-10-29T17:54:23Z

update:
Sync with team on previous angular related issue #4389

This issue due to the get and set accessors in amibient context (e.g. d.ts file) is no longer a concern as Amplify is already using TS 3.7.7 and is generating declaration file with getter and setter.
Angular 9.1 supports TS3.8 https://blog.angular.io/version-9-1-of-angular-now-available-typescript-3-8-faster-builds-and-more-eb292f989428 and we use
Our angular ui components has its own typescript build https://github.com/aws-amplify/amplify-js/blob/main/packages/amplify-ui-angular/package.json#L46 (3.2.4).

To further verify it works, I have trigger a circle CI integ test on a separate branch (deploy/release job is excluded from the workflow)

https://app.circleci.com/pipelines/github/aws-amplify/amplify-js/9236/workflows/fce0c955-0634-4ab3-966a-306c6d42b95e

4 integ tests are failing due to the ts syntax change introduced. Will look more into it.

hkjpotato · 2021-10-29T18:19:13Z

They both failed due to the fact that the declaration include "import/export type" syntax

This is because the testing environment is using a tsc version which cannot recognize this declaration file. Will look more into

If it is ok to exclude this syntax in declaration file.
If it is ok to upgrade the tsc used by testing.

hkjpotato · 2021-11-02T21:38:40Z

packages/api/src/types/index.ts

+ * This will be removed in future release when CLI and customers moves to recommeneded import styles.
+ */
+export {
+	graphqlOperation,


I am not sure why this file shows up on the diff, nothing should be changed

That is strange but can we update the copyright date :)

sammartinez · 2021-11-02T21:47:46Z

LGTM, we need to discuss with the team the impact this will have with Angular

LGTM, Angular 9+ supports TS 3.7+.

@hkjpotato can we double check old yo components as well if some customers are using them. May have some issues if the angular cli is around 7

hkjpotato · 2021-11-02T21:53:31Z

LGTM, we need to discuss with the team the impact this will have with Angular

LGTM, Angular 9+ supports TS 3.7+.

@hkjpotato can we double check old yo components as well if some customers are using them. May have some issues if the angular cli is around 7

Yep good call. Not sure if you can access these: https://app.circleci.com/pipelines/github/aws-amplify/amplify-js?branch=kj%2Fts38integ (A bunch of failure trials on angular, because the sample test is using ts3.7. The most recent one passes finally)
Angular CLI 7 is the reason why we introduce the "typesVersions" to support existing customers (they can still use ts3.7)

ashika01

Looks good to me 🚢

calebpollman

LGTM, left a minor non-blocking comment

calebpollman · 2021-11-03T02:07:49Z

package.json

@@ -71,7 +73,7 @@
 		"jest-config": "24.8.0",
 		"json-loader": "^0.5.7",
 		"lerna": "^3.13.1",
-		"prettier": "^1.19.0",
+		"prettier": "^2.4.1",
 		"pretty-quick": "^1.11.1",


Should we update pretty-quick along with prettier?

I haven't tested its newer version but from the current version's peerDependencies, it works with prettier >=1.8.0, so I think it might be safer to keep it now.

packages/api/src/types/index.ts

sammartinez

LGTM 🌮 Couple small nits but Non-Blocking to merge. Great work KJ!

elorzafe

LGTM, Thanks @hkjpotato

Can you check @calebpollman and @sammartinez comments?

Co-authored-by: Sam Martinez <samlmar@amazon.com>

hkjpotato · 2021-11-03T22:04:41Z

LGTM, Thanks @hkjpotato

Can you check @calebpollman and @sammartinez comments?

Thank you and I have checked the comments. Will discuss offline to see if we want to merge this with tomorrow's release.

wlee221 · 2021-11-04T18:01:16Z

Angular CLI 7 is the reason why we introduce the "typesVersions" to support existing customers (they can still use ts3.7)

LGTM, thanks for the research @hkjpotato.

github-actions · 2022-11-09T02:14:42Z

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

upgrade ts to 3.8 to use export type syntax

3906dec

hkjpotato changed the title ~~upgrade ts to 3.8 to use export type syntax~~ build: upgrade ts to 3.8 to use export type syntax Oct 28, 2021

hkjpotato requested a review from elorzafe October 28, 2021 14:58

hkjpotato marked this pull request as ready for review October 28, 2021 14:58

hkjpotato requested a review from iartemiev as a code owner October 28, 2021 14:58

hkjpotato marked this pull request as draft October 28, 2021 15:08

update prettier to 2.4.2 to avoid conflict with the local version in rn

80d3ec7

hkjpotato requested a review from ashika01 October 28, 2021 19:18

fix prettier conflict with nohoist

da61bdd

fix analytics jest test config by esModuleInterop

16e0512

hkjpotato added 3 commits November 2, 2021 12:43

add ts3.7 backward compatibility

7078fdf

update to use direct file reference

a3d66e7

undo unrequired type change

06632b1

hkjpotato commented Nov 2, 2021

View reviewed changes

hkjpotato marked this pull request as ready for review November 2, 2021 21:41

ashika01 approved these changes Nov 2, 2021

View reviewed changes

calebpollman approved these changes Nov 3, 2021

View reviewed changes

Merge branch 'main' into kj/ts-upgrade

b5acb76

sammartinez reviewed Nov 3, 2021

View reviewed changes

packages/api/src/types/index.ts Outdated Show resolved Hide resolved

sammartinez approved these changes Nov 3, 2021

View reviewed changes

elorzafe approved these changes Nov 3, 2021

View reviewed changes

Update packages/api/src/types/index.ts

8596633

Co-authored-by: Sam Martinez <samlmar@amazon.com>

Merge branch 'main' into kj/ts-upgrade

b87e585

ashika01 added Build Related to build issues Core Related to core Amplify issues labels Nov 4, 2021

wlee221 approved these changes Nov 4, 2021

View reviewed changes

Merge branch 'main' into kj/ts-upgrade

5cc9a92

hkjpotato merged commit d55d797 into main Nov 8, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022

jimblanc deleted the kj/ts-upgrade branch November 23, 2022 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: upgrade ts to 3.8 to use export type syntax #9122

build: upgrade ts to 3.8 to use export type syntax #9122

hkjpotato commented Oct 28, 2021 •

edited

elorzafe commented Oct 28, 2021

hkjpotato commented Oct 28, 2021

wlee221 commented Oct 28, 2021

ashika01 commented Oct 28, 2021

hkjpotato commented Oct 28, 2021 •

edited

ashika01 commented Oct 28, 2021

codecov-commenter commented Oct 28, 2021 •

edited

hkjpotato commented Oct 29, 2021

hkjpotato commented Oct 29, 2021

hkjpotato Nov 2, 2021

sammartinez Nov 3, 2021

sammartinez commented Nov 2, 2021

hkjpotato commented Nov 2, 2021

ashika01 left a comment

calebpollman left a comment

calebpollman Nov 3, 2021

hkjpotato Nov 3, 2021

sammartinez left a comment

elorzafe left a comment

hkjpotato commented Nov 3, 2021

wlee221 commented Nov 4, 2021

github-actions bot commented Nov 9, 2022

build: upgrade ts to 3.8 to use export type syntax #9122

build: upgrade ts to 3.8 to use export type syntax #9122

Conversation

hkjpotato commented Oct 28, 2021 • edited

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

elorzafe commented Oct 28, 2021

hkjpotato commented Oct 28, 2021

wlee221 commented Oct 28, 2021

ashika01 commented Oct 28, 2021

hkjpotato commented Oct 28, 2021 • edited

ashika01 commented Oct 28, 2021

codecov-commenter commented Oct 28, 2021 • edited

Codecov Report

hkjpotato commented Oct 29, 2021

hkjpotato commented Oct 29, 2021

hkjpotato Nov 2, 2021

Choose a reason for hiding this comment

sammartinez Nov 3, 2021

Choose a reason for hiding this comment

sammartinez commented Nov 2, 2021

hkjpotato commented Nov 2, 2021

ashika01 left a comment

Choose a reason for hiding this comment

calebpollman left a comment

Choose a reason for hiding this comment

calebpollman Nov 3, 2021

Choose a reason for hiding this comment

hkjpotato Nov 3, 2021

Choose a reason for hiding this comment

sammartinez left a comment

Choose a reason for hiding this comment

elorzafe left a comment

Choose a reason for hiding this comment

hkjpotato commented Nov 3, 2021

wlee221 commented Nov 4, 2021

github-actions bot commented Nov 9, 2022

hkjpotato commented Oct 28, 2021 •

edited

hkjpotato commented Oct 28, 2021 •

edited

codecov-commenter commented Oct 28, 2021 •

edited