Fix missing clip-selector in package, refactor builds #344
Conversation
Renaming to avoid confusion with build scripts.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Co-authored-by: Gary Katsevman <git@gkatsev.com>
| @@ -50,6 +47,7 @@ | |||
| "@web/test-runner": "^0.13.20", | |||
| "esbuild": "^0.14.23", | |||
| "eslint": "^8.18.0", | |||
| "globby": "^11.0.1", | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
targetwithout 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
--minifyinbuild: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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
Okidoki. Since it's a substantively smaller tweak I'm closing this PR and opened revised PR #351.
Per muxinc#344 esbuild's CLI is preferred for builds but does not currently support deep dir globs, so a third `/*` is required to reach the missing extras.
This PR restores
<media-clip-selector>to package builds. It'd fallen out because the esbuild entry pointssrc/js/*.js src/js/*/*.jsdon't encompasssrc/js/extras/media-clip-selector/index.js.Glob
/**/isn't directly supported by esbuild. A recommended alternative is to use the script API, which is what I've done here. (I choseglobbyfor the glob resolution since it's already a subdependency.)Alternative: After writing this I noticed that glob
/**/support is actually coming to esbuild via evanw/esbuild#2508. I decided to file this PR anyway since esbuild's script API benefits from TypeScript and may be preferable to the CLI.