Skip to content
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

yarn react build|dev #195

Merged
merged 25 commits into from
Aug 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
fc01355
Move react/styles.css into src/
ericclemmons Aug 18, 2021
f417619
No longer compile packages/ with Next.js
ericclemmons Aug 18, 2021
7c6752a
Use esnext import/export instead of module.exports/require
ericclemmons Aug 18, 2021
252603f
Target ES6/2015 instead of ES5
ericclemmons Aug 18, 2021
9c5f754
Mirror node's moduleResolution
ericclemmons Aug 18, 2021
8a2ee83
Inline @aws-amplify/ui-core & @aws-amplify/ui-theme-base into package
ericclemmons Aug 18, 2021
089024b
Build using tsup
ericclemmons Aug 18, 2021
3cbd9cc
Build styles.css
ericclemmons Aug 18, 2021
840b627
Inject react-shim
ericclemmons Aug 18, 2021
66811ff
Remove @fontsource/inter, since it can't be bundled into JS
ericclemmons Aug 18, 2021
24e36ee
Update yarn.lock
ericclemmons Aug 18, 2021
3e6be6c
Fix .page.tsx suffix for next-example
ericclemmons Aug 18, 2021
c280c22
Move tsup to root so it can be patched
ericclemmons Aug 20, 2021
5738715
Patch tsup at workspace root
ericclemmons Aug 20, 2021
f51ed18
Build theme-base && core && react on install
ericclemmons Aug 20, 2021
86b0a76
Remove core/index.ts
ericclemmons Aug 20, 2021
45b6751
Don't clear dist on dev
ericclemmons Aug 20, 2021
c8dda63
Only watch src
ericclemmons Aug 20, 2021
121f140
Don't inline core or theme-base because of CJS/require gets pulled in
ericclemmons Aug 20, 2021
9f6b965
Remove inspect from core
ericclemmons Aug 20, 2021
e796e25
Working core build
ericclemmons Aug 20, 2021
b09563a
Clarify how to build & watch each package
ericclemmons Aug 20, 2021
f4d7e5c
Use .cjs for ES5 files
ericclemmons Aug 20, 2021
17ad45c
Merge branch 'main' into tsup
ericclemmons Aug 20, 2021
11c53d1
Merge branch 'main' into tsup
ericclemmons Aug 20, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,17 @@
Internally, this content is served by a single, Next.js [optional catch all route](https://nextjs.org/docs/routing/dynamic-routes#optional-catch-all-routes):
[`docs/src/pages/[[...slugs]].tsx`](docs/src/pages/[[...slugs]].tsx).

## Next/React Development
## `@aws-amplify/core`

- `yarn core build` to rebuild for production.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention yarn core build --watch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

- `yarn core dev` to watch & rebuild for development.

## `@aws-amplify/ui-react`

- `yarn react build` to rebuild for production.
- `yarn react dev` to watch & rebuild for development.

## Next Examples Development

1. Create or Update an example at [`examples/...`](examples)

Expand All @@ -43,7 +53,7 @@ Internally, this content is served by a single, Next.js [optional catch all rout

Next.js should automatically hot-reload your changes in the example.

### Documentation & Testing
### E2E Testing

1. Create or Update a `${feature}.feature` file (using [Gherkin](https://cucumber.io/docs/gherkin/reference/)) describing the behavior in [`packages/e2e/cypress/integration/${slug}`](packages/e2e/cypress/integration).

Expand All @@ -61,6 +71,7 @@ Internally, this content is served by a single, Next.js [optional catch all rout
```

1. Create or Update the accompanying `${slug}.feature` tests (e.g. `packages/e2e/cypress/integration/${slug}/${feature}/${feature}.ts`
1. Start one of the [examples](examples).
1. Run `yarn e2e dev` to load Cypress
1. Click on your updated `${feature}.feature` file to validate your changes
1. Add tags (e.g. `@react`, `@vue`, `@angular`, `@skip`, or `@focus`) above your `Scenario` to indicate which platform(s) to test against
Expand Down
8 changes: 4 additions & 4 deletions docs/next.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh so nice

path.dirname(require.resolve('@aws-amplify/ui-react/package.json')),
path.dirname(require.resolve('@aws-amplify/ui-theme-base/package.json')),
path.dirname(require.resolve('amplify-docs/package.json')),
path.dirname(
// `amplify-docs` aren't bundled, so they require post-processing
require.resolve('amplify-docs/package.json')
),
],
test: /\.(js|ts)x?/,
});
Expand Down
10 changes: 2 additions & 8 deletions examples/next/next.config.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
// TODO Remove this once @aws-amplify/ui-react is compiled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huzzah

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing TODOs feels great!

const withCompileNodeModules = require('@moxy/next-compile-node-modules')({
include: /[\\/]packages[\\/]/,
test: /\.(js|ts)x?/,
});

module.exports = withCompileNodeModules({
module.exports = {
reactStrictMode: true,
pageExtensions: ['page.tsx'],
});
};
1 change: 0 additions & 1 deletion examples/next/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
},
"dependencies": {
"@aws-amplify/ui-react": "0.0.1",
"@moxy/next-compile-node-modules": "^2.1.1",
"@types/node": "^15.12.4",
"@types/react": "^17.0.11",
"next": "latest",
Expand Down
File renamed without changes.
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to replace this with turbo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"postinstall": "patch-package",
"build": "yarn workspaces run build",
"angular": "yarn workspace @aws-amplify/ui-angular",
Expand Down Expand Up @@ -37,7 +37,8 @@
},
"dependencies": {
"patch-package": "^6.4.7",
"postinstall-postinstall": "^2.1.0"
"postinstall-postinstall": "^2.1.0",
"tsup": "^4.14.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
"devDependencies": {
"husky": ">=6",
Expand Down
1 change: 0 additions & 1 deletion packages/core/index.ts

This file was deleted.

15 changes: 10 additions & 5 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,17 @@
"private": true,
"name": "@aws-amplify/ui-core",
"version": "0.0.1",
"module": "index.ts",
"main": "dist/index.js",
"type": "module",
"main": "dist/index.cjs",
"module": "dist/index.js",
"types": "dist/index.d.ts",
"files": [
"dist/*.{js,ts}",
"dist/**/*.{js,ts}"
"dist"
],
"scripts": {
"build": "tsc",
"prebuild": "rimraf dist",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

"build": "tsup --minify",
"dev": "tsup --watch src",
"test": "echo \"Skipped: no test specified\""
},
"dependencies": {
Expand All @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
9 changes: 9 additions & 0 deletions packages/core/src/globals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* This shim exists for `esbuild` to `inject` into every .tsx file.
* Otherwise, `import * as React from 'react'` would need to be added manually
*
* See: https://esbuild.github.io/content-types/#auto-import-for-jsx
* See: https://github.com/egoist/tsup/issues/390
*/

// No shims needed for core
10 changes: 10 additions & 0 deletions packages/core/tsup.config.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* @type {import("tsup").Options}
*/
module.exports = {
dts: true,
entryPoints: ['src/index.ts'],
format: ['cjs', 'esm'],
sourcemap: 'both',
splitting: false,
};
1 change: 0 additions & 1 deletion packages/react/index.tsx

This file was deleted.

File renamed without changes.
22 changes: 16 additions & 6 deletions packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,21 @@
"private": true,
"name": "@aws-amplify/ui-react",
"version": "0.0.1",
"main": "dist/index.js",
"type": "module",
"main": "dist/index.cjs",
"module": "dist/index.js",
"exports": {
".": "./dist/index.js",
"./styles.css": "./dist/index.css"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Neat, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat

},
"types": "dist/index.d.ts",
"files": [
"dist/*.{js,ts}",
"dist/**/*.{js,ts}"
"dist"
],
"scripts": {
"build": "tsc",
"prebuild": "rimraf dist",
"build": "tsup --minify",
"dev": "tsup --watch src",
"update:icons": "node scripts/updateIcons.js",
"build:icons": "yarn run clean:icons && node scripts/generateIcons.js",
"clean": "rimraf dist",
Expand All @@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"@xstate/react": "^1.4.0",
"autoprefixer": "^10.3.1",
"classnames": "^2.3.1",
"postcss-js": "^3.0.3",
"qrcode": "^1.4.4"
},
"peerDependencies": {
"@aws-amplify/ui-core": "^0.0.1",
"@aws-amplify/ui-theme-base": "^0.0.1"
"aws-amplify": "3.x.x || 4.x.x",
"react": ">= 16.8.0",
"react-dom": ">= 16.8.0"
Comment on lines +40 to +42
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we don't bundle dependencies.

},
"workspaces": {
"nohoist": [
Expand Down
10 changes: 10 additions & 0 deletions packages/react/src/globals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* This shim exists for `esbuild` to `inject` into every .tsx file.
* Otherwise, `import * as React from 'react'` would need to be added manually
*
* See: https://esbuild.github.io/content-types/#auto-import-for-jsx
* See: https://github.com/egoist/tsup/issues/390
*/

import * as React from 'react';
export { React };
Comment on lines +9 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

2 changes: 2 additions & 0 deletions packages/react/src/index.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import './styles.css';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export * from './components';
export * from './hooks';
export * from './primitives';
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/styles.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@import './components/Authenticator/styles.css';
@import './primitives/styles.css';
2 changes: 0 additions & 2 deletions packages/react/styles.css

This file was deleted.

6 changes: 4 additions & 2 deletions packages/react/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
{
"compilerOptions": {
"moduleResolution": "node",
"baseUrl": ".",
Comment on lines +3 to +4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"target": "es2015"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
"exclude": ["node_modules"],
"include": ["src/**/*.ts", "src/**/*.tsx"]
Expand Down
10 changes: 10 additions & 0 deletions packages/react/tsup.config.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* @type {import("tsup").Options}
*/
module.exports = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dts: true,
entryPoints: ['src/index.tsx'],
format: ['cjs', 'esm'],
sourcemap: 'both',
splitting: false,
};
3 changes: 0 additions & 3 deletions packages/theme-base/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,5 @@
"rimraf": "^3.0.2",
"sass": "^1.35.2",
"style-dictionary": "^3.0.1"
},
"dependencies": {
"@fontsource/inter": "^4.5.0"
Comment on lines -27 to -29
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

}
}
2 changes: 0 additions & 2 deletions packages/theme-base/src/css/theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
*/

@import '../../dist/variables.scss';
@import '~@fontsource/inter/index.css';
@import '~@fontsource/inter/variable.css';
Comment on lines -6 to -7
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


html {
font-family: var(--amplify-fonts-default-static);
Expand Down
12 changes: 12 additions & 0 deletions patches/tsup+4.14.0.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/node_modules/tsup/dist/index.js b/node_modules/tsup/dist/index.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index 1fd3ea9..8f84cf0 100644
--- a/node_modules/tsup/dist/index.js
+++ b/node_modules/tsup/dist/index.js
@@ -13633,6 +13633,7 @@ async function runEsbuild(options, { format, css }) {
jsxFactory: options.jsxFactory,
jsxFragment: options.jsxFragment,
sourcemap: options.sourcemap,
+ inject: ['./src/globals.js'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target: options.target === "es5" ? "es2016" : options.target,
plugins: [
externalPlugin({
Loading