yarn react build|dev #195

ericclemmons · 2021-08-18T21:01:21Z

❯ yarn react dev
yarn run v1.22.10
$ yarn workspace @aws-amplify/ui-react dev
$ rimraf dist
$ tsup --watch src --watch ../core/src
 CLI  Building entry: src/index.tsx
 CLI  Using tsconfig: /Users/ecclemm/Projects/aws-amplify/amplify-ui/packages/react/tsconfig.json
 CLI  tsup v4.14.0
 CLI  Using tsup config: /Users/ecclemm/Projects/aws-amplify/amplify-ui/packages/react/tsup.config.cjs
 CLI  Running in watch mode
 CLI  Target: node12
 CJS  Build start
 ESM  Build start
 ESM  Build success in 6788ms
 CJS  Build success in 7019ms
 CLI  Watching for changes in "src" | "../core/src"
 CLI  Ignoring changes in "**/{.git,node_modules}/**" | "dist"
 DTS  Build start
 DTS  Build success in 28272ms
 CLI  Change detected: change ../core/src/authService.ts
 CJS  Build start
 ESM  Build start
 CJS  Build success in 1408ms
 ESM  Build success in 1407ms

https://esbuild.github.io/content-types/#auto-import-for-jsx

ericclemmons · 2021-08-18T21:02:02Z

packages/react/package.json

+  "module": "dist/index.js",
+  "exports": {
+    ".": "./dist/index.js",
+    "./styles.css": "./dist/index.css"


import "@aws-amplify/ui-react/styles.css" correctly resolves to dist/index.css.

Neat, right?

ericclemmons · 2021-08-18T21:04:58Z

packages/react/package.json

+    "@aws-amplify/ui-core": "^0.0.1",
+    "@aws-amplify/ui-theme-base": "^0.0.1",


Build tools like microbundle, tsup, etc. externalize dependencies and inline devDependencies.

This is helpful for using packages that we don't want to publish.

Broader question: do we want to publish ui-theme-base? I noticed we publish @aws-amplify/ui which is kind of similar to what ui-theme-base is now. I don't have a strong opinion, just wanted to bring it up. Should we rename ui-theme-base to just ui?

No, any cross-platform/framework dependencies (like the state machine, design tokens, or even specs (we ran into an issue with that!)) ideally lives in @aws-amplify/ui-core.

But you're right to ask about @aws-amplify/ui.

ui-core would be a new package, but I don't think it makes sense to have. ui-core is kinda our equivalent of ui, so you're right – maybe we rename & merge ui-core and theme-base into ui.

The only thing that I found weird is aws-amplify installs it by default:
https://github.com/aws-amplify/amplify-js/blob/da8ef238928fdcad7ab307350ffff8448eec5b26/packages/aws-amplify/package.json#L47

But I have no idea why, or see the need for it.

The more I type it out, the more it makes sense for @aws-amplify/ui to contain our core/shared assets and @aws-amplify/ui-* be framework-specific stuff. core just seemed to be common convention in the ecosystem (e.g. @babel/core).

Let's queue this up for another cleanup task.

ericclemmons · 2021-08-18T21:05:10Z

packages/react/patches/tsup+4.14.0.patch

+       jsxFactory: options.jsxFactory,
+       jsxFragment: options.jsxFragment,
+       sourcemap: options.sourcemap,
+      inject: ['./src/react-shim.js'],


https://esbuild.github.io/content-types/#auto-import-for-jsx

ericclemmons · 2021-08-18T21:05:40Z

packages/react/src/index.tsx

@@ -1,3 +1,5 @@
+import './styles.css';


This is really nice – building (via PostCSS) styles as part of a single package & process.

Dumb question: if we import the styles here, developers using the React components would no longer need to import the styles like import '@aws-amplify/ui-react/styles.css' anymore? And would developers be able to get un-styled React components?

Somehow the SCSS stuff just worked?? I need to check the docs to be sure...

Good question @dbanksdesign! No, import '@aws-amplify/ui-react/styles.css' is still required.

What import does here is allow the bundler (tsup, microbundle, whatever) automatically output both JS & CSS with a single process. (Compared to having to watch CSS and JS separately)

This is consistent with how, say, an application is bundled with webpack: even though your app has JS, CSS, SVGs, PNGs, whatever, the bundler identifies all dependencies and outputs them based on your configuration.

ericclemmons · 2021-08-18T21:11:47Z

packages/theme-base/package.json

-  },
-  "dependencies": {
-    "@fontsource/inter": "^4.5.0"


The bundler had issues with WOFF files inside of this dependency.

Admittedly, this is weird to me as part of a library. Because fonts require asset files (e.g. .woff, .ttf, etc.), these have to be bundled by the client app.

Normally, I see styles only specify font-family and leave it to the consumer to determine where the fonts are located:

https://material-ui.com/customization/typography/#self-hosted-fonts

https://tailwindui.com/documentation#optional-add-the-inter-font-family

https://create-react-app.dev/docs/adding-images-fonts-and-files/

https://github.com/csstools/postcss-font-magician

Because fonts can be very expensive, I think we should revisit #165 and try to make bundle-size as much of an opt-in as possible, like https://tailwindui.com/documentation#optional-add-the-inter-font-family.

If a customer doesn't want it, font-family will naturally go to the next available font.

If a customer wants to load it in a blocking way, they can use <link>.

If a customer wants to load it in a non-blocking way, they can do what Next.js does:

https://nextjs.org/docs/basic-features/font-optimization

ericclemmons · 2021-08-20T18:34:55Z

package.json

@@ -1,7 +1,7 @@
 {
  "private": true,
  "scripts": {
-    "install": "yarn run theme-base build:all",
+    "install": "yarn run theme-base build:all && yarn core build && yarn react build",


Going to replace this with turbo

ericclemmons · 2021-08-20T18:35:43Z

package.json

@@ -37,7 +37,8 @@
  },
  "dependencies": {
    "patch-package": "^6.4.7",
-    "postinstall-postinstall": "^2.1.0"
+    "postinstall-postinstall": "^2.1.0",
+    "tsup": "^4.14.0"


tsup checked all the boxes that vite did, + .d.ts output.

Nx requires a ton of convention, which is great, but more than I wanted to change.

microbundle is slow because of rollup.

ericclemmons · 2021-08-20T18:36:10Z

packages/core/package.json

@@ -21,5 +23,8 @@
    "@types/lodash": "^4.14.170",
    "@types/webpack-env": "^1.16.2",
    "tslib": "^2.3.0"
+  },
+  "peerDependencies": {
+    "aws-amplify": "3.x.x || 4.x.x"


Otherwise our amazon-cognito-identity-js and aws-amplify dependencies get bundled!

ericclemmons · 2021-08-20T18:36:58Z

packages/react/package.json

@@ -21,15 +28,18 @@
    "test:unit:watch": "jest --watch"
  },
  "dependencies": {
+    "@aws-amplify/ui-core": "^0.0.1",
+    "@aws-amplify/ui-theme-base": "^0.0.1",


I'll move theme-base into core in another PR. We won't lose tree-shaking or anything, but it'll be 1 less bundle for customers to keep in sync.

ericclemmons · 2021-08-20T18:37:11Z

packages/react/package.json

+    "aws-amplify": "3.x.x || 4.x.x",
+    "react": ">= 16.8.0",
+    "react-dom": ">= 16.8.0"


This way we don't bundle dependencies.

ericclemmons · 2021-08-20T18:38:03Z

packages/react/src/globals.js

+import * as React from 'react';
+export { React };


Vite does this automatically. I had to patch tsup to avoid changing EVERY .tsx file.

ericclemmons · 2021-08-20T18:39:15Z

packages/react/tsconfig.json

+    "moduleResolution": "node",
+    "baseUrl": ".",


I thought this was the default, but tsup & vite weren't picking up on node_modules until I added this.

dbanksdesign

Overall, I love it. A couple small questions, nothing blocking.

dbanksdesign · 2021-08-18T21:52:53Z

docs/next.config.js

@@ -9,10 +9,10 @@ const withNextPluginPreval = require('next-plugin-preval/config')();
 const withCompileNodeModules = require('@moxy/next-compile-node-modules')({
  include: [
    // Using `path.dirname` because `package.json#main` doesn't exist in some packages yet
-    path.dirname(require.resolve('@aws-amplify/ui-core/package.json')),


ahhh so nice

dbanksdesign · 2021-08-18T21:53:27Z

examples/next/next.config.js

@@ -1,10 +1,4 @@
-// TODO Remove this once @aws-amplify/ui-react is compiled


Removing TODOs feels great!

dbanksdesign · 2021-08-18T21:56:28Z

packages/react/src/index.tsx

@@ -1,3 +1,5 @@
+import './styles.css';


Dumb question: if we import the styles here, developers using the React components would no longer need to import the styles like import '@aws-amplify/ui-react/styles.css' anymore? And would developers be able to get un-styled React components?

dbanksdesign · 2021-08-20T18:35:40Z

package.json

@@ -1,7 +1,7 @@
 {
  "private": true,
  "scripts": {
-    "install": "yarn run theme-base build:all",
+    "install": "yarn run theme-base build:all && yarn core build && yarn react build",


ohh I like, now I can just run install and then start up the docs or example next server

Exactly right @dbanksdesign – I want the majority of the packages/examples/docs/etc. to "just work" with git clone and yarn install.

The only exception is really environments/, and that's because they require an AWS account.

In the future, we can even avoid that by including aws-exports.js – we would only need to monitor those resources for usage/abuse.

ericclemmons · 2021-08-20T18:40:30Z

packages/react/tsconfig.json

    "declaration": true,
    "esModuleInterop": true,
    "jsx": "react-jsx",
    "rootDir": "./src",
    "outDir": "./dist",
    "skipLibCheck": true,
    "sourceMap": true,
-    "module": "commonjs",
-    "target": "es5"
+    "module": "esnext",


So, TIL : https://stackoverflow.com/questions/41993811/understanding-target-and-module-in-tsconfig

module basically means module.exports or import/export or import():

microsoft/TypeScript#24082

ericclemmons · 2021-08-20T18:42:14Z

packages/react/tsconfig.json

-    "module": "commonjs",
-    "target": "es5"
+    "module": "esnext",
+    "target": "es2015"


target means the language features:

https://stackoverflow.com/questions/41993811/understanding-target-and-module-in-tsconfig

Targeting evergreen browsers, this could even be esnext but I figure we'll standup more examples (CRA, Nuxt, etc.) to determine what that impact is.

But the less TS has to transpile, the faster our builds will be.

ericclemmons · 2021-08-20T18:42:36Z

packages/react/tsup.config.cjs

+/**
+ * @type {import("tsup").Options}
+ */
+module.exports = {


I did it this way for type safety vs. tsup src/index.tsx --format cjs,esm ...

ericclemmons · 2021-08-20T18:42:53Z

patches/tsup+4.14.0.patch

+       jsxFactory: options.jsxFactory,
+       jsxFragment: options.jsxFragment,
+       sourcemap: options.sourcemap,
+      inject: ['./src/globals.js'],


egoist/tsup#390

wlee221

This is looking great! Thanks @ericclemmons. Just got some suggestions/questions.

wlee221 · 2021-08-20T19:49:20Z

CONTRIBUTING.md

-## Next/React Development
+## `@aws-amplify/core`
+
+- `yarn core build` to rebuild for production.


Should we mention yarn core build --watch?

yarn core dev is the answer for local development (including watching).

(I also learned that the build tool may not always be the dev tool!)

wlee221 · 2021-08-20T19:57:01Z

examples/next/next.config.js

@@ -1,10 +1,4 @@
-// TODO Remove this once @aws-amplify/ui-react is compiled


Removing TODOs feels great!

wlee221 · 2021-08-20T20:04:59Z

packages/core/package.json

  ],
  "scripts": {
-    "build": "tsc",
+    "prebuild": "rimraf dist",


Should we have a separate yarn clean script here? I find explicit clean scripts when I'm looking at module/build issues.

Suggested change

"prebuild": "rimraf dist",

"clean": "rimraf dist",

"prebuild": "yarn clean",

You know, I went back-and-forth on this. What I noticed was that it was tough to trace when something was being used and where. (build:icons is an example of this)

Having clean explicitly defined kinda felt like I was promoting it to a public API (e.g. yarn react clean) vs. having only build|dev being the public APIs.

I hope the indirection from build => prebuild => clean is clearer with build => prebuild 🙏

(I don't have a strong opinion – I've done NPM scripts every which way since per-env)

Is there a use-case to run only a clean? I've realized whenever I'm doing a clean it's because I want to blow up the build artifacts and rebuild them, so just being part of prebuild makes sense to me. But if there are use cases for "clean and not build" I'm for it.

Great question. I don't have one. Your use-case models my experience as well:

"Build for production", which means a clean slate so we don't re-publish old artifacts.

"Dev" should not remove clean because I found out it breaks hot-reload when those references disappear.

I suppose there could be a reason to clean and do dev, but IME yarn install is basically our starting point for "setup and build everything, and I'll chose what part I'm working on".

wlee221 · 2021-08-20T20:11:26Z

patches/tsup+4.14.0.patch

@@ -0,0 +1,12 @@
+diff --git a/node_modules/tsup/dist/index.js b/node_modules/tsup/dist/index.js


TIL https://www.npmjs.com/package/patch-package

ericclemmons

Responding to comments, but somehow it's a review? ¯_(ツ)_/¯

ericclemmons · 2021-08-20T18:44:28Z

packages/theme-base/src/css/theme.scss

-@import '~@fontsource/inter/index.css';
-@import '~@fontsource/inter/variable.css';


Oh yeah, with this change we will need to update our theme instructions to add the Inter font (see above comment)

ericclemmons · 2021-08-20T20:30:36Z

CONTRIBUTING.md

-## Next/React Development
+## `@aws-amplify/core`
+
+- `yarn core build` to rebuild for production.


yarn core dev is the answer for local development (including watching).

(I also learned that the build tool may not always be the dev tool!)

ericclemmons · 2021-08-20T20:32:23Z

package.json

@@ -1,7 +1,7 @@
 {
  "private": true,
  "scripts": {
-    "install": "yarn run theme-base build:all",
+    "install": "yarn run theme-base build:all && yarn core build && yarn react build",


Exactly right @dbanksdesign – I want the majority of the packages/examples/docs/etc. to "just work" with git clone and yarn install.

The only exception is really environments/, and that's because they require an AWS account.

In the future, we can even avoid that by including aws-exports.js – we would only need to monitor those resources for usage/abuse.

ericclemmons · 2021-08-20T20:40:32Z

packages/core/package.json

  ],
  "scripts": {
-    "build": "tsc",
+    "prebuild": "rimraf dist",


You know, I went back-and-forth on this. What I noticed was that it was tough to trace when something was being used and where. (build:icons is an example of this)

Having clean explicitly defined kinda felt like I was promoting it to a public API (e.g. yarn react clean) vs. having only build|dev being the public APIs.

I hope the indirection from build => prebuild => clean is clearer with build => prebuild 🙏

(I don't have a strong opinion – I've done NPM scripts every which way since per-env)

ericclemmons · 2021-08-20T20:42:45Z

packages/react/src/index.tsx

@@ -1,3 +1,5 @@
+import './styles.css';


Good question @dbanksdesign! No, import '@aws-amplify/ui-react/styles.css' is still required.

What import does here is allow the bundler (tsup, microbundle, whatever) automatically output both JS & CSS with a single process. (Compared to having to watch CSS and JS separately)

This is consistent with how, say, an application is bundled with webpack: even though your app has JS, CSS, SVGs, PNGs, whatever, the bundler identifies all dependencies and outputs them based on your configuration.

Sync amplify-ui to amplify-ui-staging

ericclemmons added 12 commits August 18, 2021 14:28

Move react/styles.css into src/

fc01355

No longer compile packages/ with Next.js

f417619

Use esnext import/export instead of module.exports/require

7c6752a

Target ES6/2015 instead of ES5

252603f

Mirror node's moduleResolution

9c5f754

Inline @aws-amplify/ui-core & @aws-amplify/ui-theme-base into package

8a2ee83

Build using tsup

089024b

Build styles.css

3cbd9cc

Inject react-shim

840b627

https://esbuild.github.io/content-types/#auto-import-for-jsx

Remove @fontsource/inter, since it can't be bundled into JS

66811ff

Update yarn.lock

24e36ee

Fix .page.tsx suffix for next-example

3e6be6c

ericclemmons commented Aug 18, 2021

View reviewed changes

ericclemmons added 11 commits August 20, 2021 12:51

Move tsup to root so it can be patched

c280c22

Patch tsup at workspace root

5738715

Build theme-base && core && react on install

f51ed18

Remove core/index.ts

86b0a76

Don't clear dist on dev

45b6751

Only watch src

c8dda63

Don't inline core or theme-base because of CJS/require gets pulled in

121f140

Remove inspect from core

9f6b965

Working core build

e796e25

Clarify how to build & watch each package

b09563a

Use .cjs for ES5 files

f4d7e5c

ericclemmons had a problem deploying to ci August 20, 2021 18:32 Failure

ericclemmons temporarily deployed to ci August 20, 2021 18:32 Inactive

Merge branch 'main' into tsup

17ad45c

ericclemmons marked this pull request as ready for review August 20, 2021 18:33

ericclemmons commented Aug 20, 2021

View reviewed changes

ericclemmons had a problem deploying to ci August 20, 2021 18:37 Error

ericclemmons had a problem deploying to ci August 20, 2021 18:37 Failure

ericclemmons commented Aug 20, 2021

View reviewed changes

dbanksdesign approved these changes Aug 20, 2021

View reviewed changes

ericclemmons commented Aug 20, 2021

View reviewed changes

wlee221 approved these changes Aug 20, 2021

View reviewed changes

ericclemmons commented Aug 20, 2021

View reviewed changes

Merge branch 'main' into tsup

11c53d1

ericclemmons had a problem deploying to ci August 20, 2021 20:53 Failure

ericclemmons temporarily deployed to ci August 20, 2021 20:53 Inactive

ericclemmons had a problem deploying to ci August 20, 2021 20:53 Error

ericclemmons had a problem deploying to ci August 20, 2021 21:36 Failure

ericclemmons temporarily deployed to ci August 20, 2021 21:36 Inactive

ericclemmons merged commit 6970069 into main Aug 20, 2021

ericclemmons deleted the tsup branch November 30, 2021 19:37

thaddmt pushed a commit that referenced this pull request Apr 7, 2023

Merge pull request #195 from aws-amplify/pr/repo-sync

cd1da27

Sync amplify-ui to amplify-ui-staging

		"@aws-amplify/ui-core": "^0.0.1",
		"@aws-amplify/ui-theme-base": "^0.0.1",

		@@ -1,10 +1,4 @@
		// TODO Remove this once @aws-amplify/ui-react is compiled

	"prebuild": "rimraf dist",
	"clean": "rimraf dist",
	"prebuild": "yarn clean",

		@@ -0,0 +1,12 @@
		diff --git a/node_modules/tsup/dist/index.js b/node_modules/tsup/dist/index.js

		@import '~@fontsource/inter/index.css';
		@import '~@fontsource/inter/variable.css';

yarn react build|dev #195

yarn react build|dev #195

Conversation

ericclemmons commented Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanksdesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlee221 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericclemmons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericclemmons commented Aug 18, 2021 •

edited

Loading