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.

Sign up for GitHub

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

Fix missing clip-selector in package, refactor builds #344

Closed
wants to merge 3 commits into from
Closed

Fix missing clip-selector in package, refactor builds #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bafdaa
Move esbuild.js to serve.js
redoPop Oct 19, 2022
8b4aeeb
Replace CLI esbuild with script API
redoPop Oct 19, 2022
6d9564a
Fix missing minify of ESM Module build
redoPop Oct 20, 2022
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
12 changes: 5 additions & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,11 @@
"format": "prettier --write .",
"clean": "rimraf dist",
"lint": "eslint src/js",
"build:esm": "esbuild src/js/*.js src/js/*/*.js --target=es2019 --format=esm --outdir=dist",
"build:esm-module": "esbuild src/js/index.js --target=es2019 --format=esm --bundle --minify --sourcemap --outfile=dist/media-chrome.mjs",
"postbuild:esm-module": "yarn size",
"build:cjs": "esbuild src/js/*.js src/js/*/*.js --target=es2019 --format=cjs --outdir=dist/cjs",
"build:iife": "esbuild src/js/index.js --bundle --target=es2019 --format=iife --outdir=dist/iife --minify --sourcemap --global-name=MediaChrome",
"build:esbuild": "node ./scripts/build.js",
"postbuild:esbuild": "yarn size",
"build:react": "node ./scripts/react/build.js",
"build": "yarn build:esm && yarn build:cjs && yarn build:iife && yarn build:react && yarn build:esm-module",
"dev": "node ./scripts/esbuild.js",
"build": "yarn build:esbuild && yarn build:react",
"dev": "node ./scripts/serve.js",
"start": "yarn dev",
"test": "web-test-runner --coverage --config test/web-test-runner.config.js",
"publish-release": "./scripts/publish.sh",
Expand Down Expand Up @@ -50,6 +47,7 @@
"@web/test-runner": "^0.13.20",
"esbuild": "^0.14.23",
"eslint": "^8.18.0",
"globby": "^11.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

@redoPop thanks for the contribution!

I'd lean more towards keeping the NPM scripts intact and not add an extra dependency globby
could it work by adding an extra input for media-clip-selector to the NPM scripts?
src/js/extra/*/*.js

the main reason we opted for a JS script for yarn dev was that it required a 404 page and redirect which esbuild didn't offer out of the box

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, an entry point of src/js/*.js src/js/*/*.js src/js/*/*/*.js would work to preserve use of the CLI in package.json (all 3 entry points would be needed for now).

Before I go ahead with that change, I'd like to make a quick pitch for some advantages to esbuild's JS API:

  • We don't have to use globby or another dep, and I'm happy to update this PR so you can see how it'd look dep-free – I only chose the globby shortcut because it's a subdep via @web/test-runner
  • esbuild.build() is TypeScripted, which makes it easier to adjust build configurations with IntelliSense
  • We can use a base object to share common build configurations via the spread operator, e.g. to lock the target without having to manually change each of the build configs
  • Object configs are often easier to read than run-on commands in package.json scripts, e.g. Gary pointed out that I'd overlooked a --minify in build:esm-module
  • As the project grows, we can take advantage of async builds to improve performance (would only save a few ms right now, so I chose to await each build to preserve the familiar build order for this PR)

No strong feelings on this, and I certainly understand there are other concerns and priorities – just wanted to put in a quick word for it before reverting as I've found it helpful in other projects!

Want to see how a dep-free JS version would look before deciding, or shall we go ahead with the extra entry point in npm scripts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@redoPop thanks, for now CLI scripts will be sufficient I think.
maybe if we would add more bundles in the future it could become easier but 4 is still ok.

we're also planning to move to jsdelivr for the CDN link install and it might mean we can remove the
esm-module bundle because jsdelivr nicely bundles and minifies it. unpkg doesn't do this I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

unpkg doesn't bundle but can convert bare esm into full urls

?module

Expands all “bare” import specifiers in JavaScript modules to unpkg URLs. This feature is very experimental

from https://unpkg.com/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okidoki. Since it's a substantively smaller tweak I'm closing this PR and opened revised PR #351.

"prettier": "^2.5.1",
"react": "^17.0.2",
"rimraf": "^3.0.2",
Expand Down
46 changes: 46 additions & 0 deletions scripts/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import esbuild from 'esbuild';
import globby from 'globby';

const baseConfig = {
target: 'es2019',
logLevel: 'info',
};

// ESM
await esbuild.build({
...baseConfig,
entryPoints: await globby(['src/js/**/*.js']),
format: 'esm',
outdir: 'dist',
});

// CJS
await esbuild.build({
...baseConfig,
entryPoints: await globby(['src/js/**/*.js']),
format: 'cjs',
outdir: 'dist/cjs',
});

// IIFE
await esbuild.build({
...baseConfig,
entryPoints: ['src/js/index.js'],
format: 'iife',
bundle: true,
minify: true,
sourcemap: true,
outdir: 'dist/iife',
globalName: 'MediaChrome',
});

// ESM Module
await esbuild.build({
...baseConfig,
entryPoints: ['src/js/index.js'],
format: 'esm',
bundle: true,
redoPop marked this conversation as resolved.
Show resolved Hide resolved
minify: true,
sourcemap: true,
outfile: 'dist/media-chrome.mjs',
});
0 scripts/esbuild.js → scripts/serve.js
View file
Open in desktop
File renamed without changes.