feat(repo): set up rollup to emit esm and cjs artifacts #12522

HuiSF · 2023-11-07T02:08:00Z

Description of changes

Set up roll for emitting ESM and CJS artifacts of the packages

Notable Changes to The Packages Besides the Rollup Set Up

`packages/auth`

fix(auth): resolve circular dependencies deadlock
Rollup generates extra modules for credentialsProvider due to unresolvable circular dependencies.

`packages/storage`

fix(storage): remove unnecessary package.json files
fix(storage): xmlParser browser module resolution
fix(storage): fix fetch xhr override for browser
fix(storage): fix runtime module resolution
Following the Node ESM, the transformed files include fully resolved module import path. With this the original Storage runtime package.json module resolution stopped working. To resolve this, this PR moved runtime module resolution into the package root package.json using the fully resolved module paths.

Issue #, if available

Description of how you validated changes

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.

github-actions · 2023-11-07T02:09:16Z

⚠️ This PR includes manual changes to the "aws-amplify" package.json file, which can have library-wide implications.

Please ensure that this PR:

Does not modify "@aws-amplify/*" dependency versions, which may misalign core dependencies across the library.

A repository administrator is required to review & merge this change.

jimblanc

Looking good! Some suggestions & questions below. We should merge & squash this when the time comes to keep things tidy

jimblanc · 2023-11-07T15:23:53Z

packages/adapter-nextjs/__tests__/api/generateServerClient.test.ts

@@ -10,7 +10,7 @@ import {
 import { NextApiRequestMock, NextApiResponseMock } from '../mocks/headers';
 import { createServerRunnerForAPI } from '../../src/api/createServerRunnerForAPI';

-const headers = import('next/headers');
+const headers = import('next/headers.js');


Reviewer context: Required because Next.js vends ESM artifacts as JS (same with Lodash)

jimblanc · 2023-11-07T15:44:29Z

packages/adapter-nextjs/rollup.config.mjs

+		plugins: [
+			typescript({
+				...cjsTSOptions,
+				tsconfig: 'tsconfig.build.json',


Do we want to use tsconfig.json or tsconfig.build.json for this package? In other packages I believe tsconfig.build.json was only used for the ts-coverage check and has been getting removed

I think we should the common pattern

tsconfig.json for dev

tsconfig.build.json for build

We can apply this pattern in a separate PR though.

packages/adapter-nextjs/tsconfig.build.json

packages/api/package.json

packages/analytics/package.json

packages/react-native/rollup.config.mjs

packages/storage/package.json

packages/storage/server/package.json

AllanZhengYP · 2023-11-07T18:18:55Z

packages/api-rest/package.json

+		"@rollup/plugin-typescript": "11.1.5",
+		"rollup": "3.29.4",


Can these be moved to the workspace dev dependencies

Can't do unfortunately, until we are able use a consistent typescript version in the workspace, and get rid of the 0.x version of rollup from the workspace.

The TypeScript version are fine to align. And workspace rollup version is for building CDN artifacts that TBH should not be part of the package build script. We can take a followup item to streamline this

AllanZhengYP

Thank you for this much needed change @HuiSF I don't have any blocking comments. 🚀🚀🚀

packages/storage/server/package.json

packages/adapter-nextjs/rollup.config.mjs

packages/adapter-nextjs/tsconfig.build.json

AllanZhengYP · 2023-11-07T18:56:22Z

packages/adapter-nextjs/src/internals/index.ts

@@ -1,5 +0,0 @@
-// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


Nit: Any context on removing the internals folder?

It was required originally by GraphQL client server APIs, but these APIs are now moved into adapter-next/api. The internals are no longer needed.

AllanZhengYP · 2023-11-07T19:13:12Z

packages/api-graphql/src/GraphQLAPI.ts

@@ -3,7 +3,7 @@
 import { AmplifyClassV6 } from '@aws-amplify/core';
 import { Category, ApiAction } from '@aws-amplify/core/internals/utils';
 import { GraphQLOptions, GraphQLResult } from './types';
-import { InternalGraphQLAPIClass } from './internals';


For review context: this is required to prevent circular dependencies.

@HuiSF Can we add a little comment here as well?

AllanZhengYP · 2023-11-07T19:34:35Z

packages/aws-amplify/webpack.config.js

-			'./lib-esm/auth/index.js',
-			'./lib-esm/auth/cognito/index.js',
-			'./lib-esm/storage/index.js',
-			'./lib-esm/storage/s3/index.js',


Non-blocking -- This list should include all the packages under the dist folder, like in v5. We can address this separately.

I think the original set up is to create a umd bundle for supported CDN script use cases. We can revisit this part separately.

AllanZhengYP · 2023-11-07T19:39:37Z

packages/datastore/ssr/package.json

@@ -1,8 +1,5 @@
 {
-	"main": "../lib/ssr/index.js",


Super nit: add a name indicating the import path.

packages/react-native/package.json

packages/rtn-push-notification/build.js

rollup/utils.mjs

elorzafe · 2023-11-07T21:19:53Z

packages/api-graphql/package.json

+	"build:cjs:watch": "rimraf dist/cjs && tsc -m commonjs --outDir dist/cjs --watch",
+	"build:esm:watch": "rimraf dist/esm && tsc -m esnext --outDir dist/esm --watch",
+	"build": "npm run clean && npm run build:esm-cjs && npm run build:umd",
+	"clean": "npm run clean:size && rimraf dist lib lib-esm",


nit: do we need to reference lib and lib-esm?

Did this just to clean up old build artifacts locally once this gets merged

elorzafe · 2023-11-07T21:31:11Z

packages/api-rest/package.json

 		"typescript": "5.0.2"
 	},
 	"size-limit": [
 		{
 			"name": "API (rest client)",
-			"path": "./lib-esm/index.js",
+			"path": "./dist/esm/index.mjs",


nit: we can remove this size-limit if is not used on this package

Sure I can clean all these up in separate PR.

elorzafe · 2023-11-07T21:32:37Z

packages/api/package.json

+		"./dist/cjs/API.js",
+		"./dist/esm/API.mjs"


@AllanZhengYP do we need this side effect?

These side effect entries have been removed at 4672524

packages/aws-amplify/package.json

elorzafe · 2023-11-07T22:11:51Z

packages/datastore/ssr/package.json

-		"../lib/ssr/index": "../lib-esm/ssr/index.js"
-	}
+	"main": "../dist/cjs/ssr/index.js",
+	"module": "../dist/esm/ssr/index.js",


should this be mjs?

DataStore is no longer support SSR, will remove this subpath.

elorzafe · 2023-11-07T22:15:39Z

packages/interactions/package.json

-			"require": "./lib/index.js"
+			"types": "./dist/esm/index.d.ts",
+			"import": "./dist/esm/index.mjs",
+			"require": "./dist/cjs/index.js"


is missing react-native here?

I think interactions doesn't support RN atm?

packages/pubsub/iot/packages.json

packages/pubsub/mqtt/packages.json

elorzafe

I found a couple of things, let me know if that helped

Co-authored-by: Jim Blanchard <[email protected]>

HuiSF requested review from a team as code owners November 7, 2023 02:08

jimblanc reviewed Nov 7, 2023

View reviewed changes

HuiSF requested a review from jimblanc November 7, 2023 18:18

AllanZhengYP reviewed Nov 7, 2023

View reviewed changes

jimblanc previously approved these changes Nov 7, 2023

View reviewed changes

HuiSF force-pushed the hui/feat/repo/rollup-esm-cjs branch from 033a710 to 180c915 Compare November 7, 2023 18:28

HuiSF dismissed jimblanc’s stale review via f5d6ebf November 7, 2023 19:38

AllanZhengYP previously approved these changes Nov 7, 2023

View reviewed changes

jimblanc previously approved these changes Nov 7, 2023

View reviewed changes

HuiSF dismissed stale reviews from jimblanc and AllanZhengYP via c31a8d1 November 7, 2023 20:25

HuiSF force-pushed the hui/feat/repo/rollup-esm-cjs branch from c31a8d1 to 4672524 Compare November 7, 2023 20:28

AllanZhengYP previously approved these changes Nov 7, 2023

View reviewed changes

jimblanc previously approved these changes Nov 7, 2023

View reviewed changes