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

build!: generate cjs and type declarations #1572

Merged
merged 7 commits into from
Apr 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ jobs:
- name: Build Dist 🔧
run: yarn run build

- name: Check for "tslib" in build # tslib is peer dep of rollup ts plugin, but we're not including tslib due to lack of need
run: |
if grep -r '--include=*.'{cjs,js} 'tslib' ./lib -q
then
echo "tslib found in lib"
exit 1
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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


storybook:
runs-on: ubuntu-latest
steps:
Expand Down
23 changes: 19 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,24 @@
"author": "CZI <edu-frontend-infra@chanzuckerberg.com>",
"homepage": "https://github.com/chanzuckerberg/edu-design-system",
"license": "MIT",
"main": "lib/index.js",
"exports": {
".": {
"import": "./lib/index.js",
"require": "./lib/index.cjs"
},
"./index.css": {
"import": "./lib/index.css",
"require": "./lib/index.css"
},
"./fonts.css": {
"import": "./lib/tokens/fonts.css",
"require": "./lib/tokens/fonts.css"
},
Comment on lines +13 to +20
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 short handed some of these styles exports so it's less unwieldy (i.e. import '@chanzuckerberg/eds/fonts.css and not import @chanzuckerberg/eds/lib/tokens/fonts.css) ... it IS breaking but the whole build feature will be breaking so might as well include it... lmk if we shouldn't break this since cost is pretty low

"./tailwind.config": {
"import": "./tailwind.config.ts"
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahuth Not sure if I'm using conditional exports right but seems that we have to explicitly state which files are allowed to be exported

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's true. I kind of forgot that there was more than just the index.js files (such as fonts, css, tailwind, etc).

Does this seem like it's too cumbersome, or is it okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine unless consumers want to use some of the internal components or the utils, which we haven't come across yet

"types": "lib/index.d.ts",
"sideEffects": [
"lib/tokens/css/variables.css",
"lib/tokens/fonts.css"
Expand All @@ -22,7 +39,7 @@
},
"files": [
"/lib",
"tailwind.config.js"
"tailwind.config.mjs"
],
"scripts": {
"build": "yarn build:clean && yarn build:tokens && yarn build:js && yarn copy-fonts-to-lib",
Expand Down Expand Up @@ -90,8 +107,6 @@
"react-popper": "^2.3.0",
"react-portal": "^4.2.2"
},
"entry": "lib/index.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.

Not sure what this line did?

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea. Probs something in the BM template that doesn't do anything.

"types": "lib/index.d.ts",
"devDependencies": {
"@chanzuckerberg/axe-storybook-testing": "^6.3.0",
"@chanzuckerberg/eslint-config-edu-js": "^1.0.3",
Expand Down
26 changes: 20 additions & 6 deletions rollup.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,28 @@ import { nodeResolve } from '@rollup/plugin-node-resolve';
import typescript from '@rollup/plugin-typescript';
import postcss from 'rollup-plugin-postcss';

/**
* @type {import('rollup').RollupOptions}
*/
export default {
input: 'src/index.ts',
output: {
dir: 'lib',
format: 'es',
preserveModules: true,
preserveModulesRoot: 'src',
},
output: [
{
dir: 'lib',
format: 'es',
preserveModules: true,
preserveModulesRoot: 'src',
sourcemap: true,
},
{
dir: 'lib',
format: 'cjs',
preserveModules: true,
preserveModulesRoot: 'src',
sourcemap: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can turn sourcemaps off, not sure if anyone will take a look but also wouldn't hurt to have

entryFileNames: '[name].cjs',
Copy link
Contributor Author

@jinlee93 jinlee93 Mar 31, 2023

Choose a reason for hiding this comment

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

This builds the cjs files using .cjs extension into the same directories, no longer requiring separate es and cjs directories... I prefer it this way but we can always go back to separate directories
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way seems reasonable, so let's go with your preference.

},
],
/**
* With the nodeResolve plugin, this marks all EDS node_modules as external, aka provided by the consumer.
* Since EDS is not imported directly into a web <script>, package managers (such as npm or yarn)
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// This brings the style tokens that are css custom variables into the built stylesheet so only one stylesheet for EDS has to be imported.
import './tokens-dist/css/variables.css';

export { default as Accordion } from './components/Accordion';
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.build.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"extends": "./tsconfig",
"include": ["src/index.ts", "src/custom.d.ts"],
"include": ["src/index.ts", "src/custom.d.ts"]
}
4 changes: 2 additions & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
"allowJs": false,
"allowSyntheticDefaultImports": true,
"declaration": true,
"declarationDir": "lib",
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"jsx": "react",
"lib": ["es5", "es6", "dom"],
"moduleResolution": "node",
"noUnusedLocals": true,
"outDir": "lib",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer using tsc to build

"resolveJsonModule": true,
"skipLibCheck": true,
"strict": true,
"suppressImplicitAnyIndexErrors": true,
"target": "es6"
"target": "es2018"
Copy link
Contributor Author

@jinlee93 jinlee93 Apr 3, 2023

Choose a reason for hiding this comment

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

Target is being bumped to remove tslib usage in running rollup build with @rollup/plugin-typescript. tslib was used for __rest utility which is the spread object functionality that comes with es2018. tslib is being grepped for as a build test

}
}