LG-5764 experiment - publishing both non-minified and minified bundles #3325
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 5cfe819 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
nima-taheri-mongodb
changed the title
nima-taheri-mongodb
changed the title
nima-taheri-mongodb
changed the title
nima-taheri-mongodb
changed the title
nima-taheri-mongodb
force-pushed
the
cloudp-352308-env-specific-bundles-experiment
branch
from
8461d7d to
e98c775
Compare
nima-taheri-mongodb
changed the title
nima-taheri-mongodb
requested review from
shaneeza and
tsck
and removed request for
a team
nima-taheri-mongodb
force-pushed
the
cloudp-352308-fix-tsc-script
branch
from
71f2868 to
82c6e29
Compare
nima-taheri-mongodb
force-pushed
the
cloudp-352308-env-specific-bundles-experiment
branch
from
e98c775 to
b9c5489
Compare
tsck
reviewed
Nov 20, 2025
tools/build/src/minify.ts
Outdated
Collaborator
There was a problem hiding this comment.
I'm wondering if minification should be part of the rollup build step rather than a separate lg-build minify command? Then where we want it generated for now we could just do so in the package rollup (charts/legend/rollup.config.mjs in this case). What are your thoughts on that?
Collaborator
Author
There was a problem hiding this comment.
Hey Terrence, thanks for taking a look. That's definitely possible, but it would require running Rollup twice once with terse plugin and once without it. I was a bit concerned about the overhead of repeating transpilation, polyfilling, and other common steps.
In this approach, I tried to copy Underscore project's build config, which uses Rollup for transpilation and runs Terser as a separate script for minification. I figured that might be a more efficient approach.
Let me know if you like the other approach better or there's a 3rd better option.
https://github.com/jashkenas/underscore/blob/master/package.json
Collaborator
There was a problem hiding this comment.
One downside to this approach is that we'd need to update every package's scripts to use lg-build bundle && lg-build minify. I'd rather not do that unless we really need to.
IMO adding the minification as a final step to buildPackage would make the most sense to me (maybe with an optional --minify or --no-minify flag for local development?)
Collaborator
There was a problem hiding this comment.
Going the complete opposite direction, we could make a new script in every package, that get s run by turbo:
"packages/<package>/package.json"
{
"scripts": {
"build": "lg-build bundle",
"minify": "lg-build minify"
}
}"package.json"
{
"scripts": {
"build": "turbo run build minify tsc"
}
}"turbo.json"
{
"tasks": {
"minify": {
"dependsOn": ["build"],
}
}
}That said, I still think I prefer combining bundling & minification into one step
Collaborator
Author
There was a problem hiding this comment.
Both resonate with me. We should generate both bundles by default, since package.json (which we’ll need to update across all packages) points to both the minified and non-minified bundles.
Collaborator
Author
There was a problem hiding this comment.
Folks, great news! While trying a POC on Adam's suggested approach I realized Terrence's proposal was possible. So a single rollup config can have multiple outputs with each output having the flexibility to add another extra plugin (e.g. terser for minification) applied to it.
I'll update the PR today. 🚀
nima-taheri-mongodb
force-pushed
the
cloudp-352308-env-specific-bundles-experiment
branch
from
b9c5489 to
e696014
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements an experimental dual-bundle publishing strategy for the @lg-charts/legend package, generating both non-minified and minified builds. The non-minified bundle becomes the default export while minified versions are provided for production browser environments, improving debugging capabilities with better source maps and readable stack traces.
Key Changes:
- Refactored Rollup configuration to support multiple output formats with optional minification
- Added production-specific conditional exports to package.json for browser environments
- Created new build tooling to handle minification as a separate step
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/build/config/rollup.config.mjs | Refactored to generate both minified and non-minified bundles through new modernDevProdConfig |
| charts/legend/rollup.config.mjs | New rollup config importing modernDevProdConfig for the experimental dual-bundle approach |
| charts/legend/package.json | Updated exports to use conditional browser/production paths for minified bundles |
| .changeset/breezy-groups-turn.md | Documents new minification commands for build tools |
| .changeset/blue-laws-tie.md | Documents legend package build configuration changes |
nima-taheri-mongodb
force-pushed
the
cloudp-352308-env-specific-bundles-experiment
branch
3 times, most recently
from
c93cec3 to
ad378a4
Compare
nima-taheri-mongodb
force-pushed
the
cloudp-352308-env-specific-bundles-experiment
branch
from
ad378a4 to
fa18914
Compare
Contributor
|
Size Change: +30.8 kB (+1.71%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
.changeset/breezy-groups-turn.md
Outdated
| '@lg-tools/cli': minor | ||
| --- | ||
|
|
||
| Introduced new `lg build-minify` and `lg-build minify` commands to separately minify JavaScript bundle files. |
Collaborator
There was a problem hiding this comment.
nit: this needs to be updated
Collaborator
Author
There was a problem hiding this comment.
Oh, right. I'll update it 🫡
Comment on lines
+44
to
+55
| "import": { | ||
| "browser": { | ||
| "production": "./dist/esm/index-min.js" | ||
| }, | ||
| "default": "./dist/esm/index.js" | ||
| }, | ||
| "require": { | ||
| "browser": { | ||
| "production": "./dist/umd/index-min.js" | ||
| }, | ||
| "default": "./dist/umd/index.js" | ||
| } |
Collaborator
There was a problem hiding this comment.
For all my complaining about updating package.json files, I missed that this is necessary to get the dev builds. At least this is optional now though.
We should open a ticket to roll this out across the codebase
Collaborator
Author
There was a problem hiding this comment.
I'll create a ticket.
It's a quick job, except icon package that needs a bit of scripting.
I have good context, I can take that on and do it in a day. (not wise to say that ever 😂)
Collaborator
Author
There was a problem hiding this comment.
Created two sub-tasks for the existing story.
- Experiment on a package | LG-5764
- Use the finalized approach on all packages | LG-5765
Comment on lines
+44
to
+48
| "import": { | ||
| "browser": { | ||
| "production": "./dist/esm/index-min.js" | ||
| }, | ||
| "default": "./dist/esm/index.js" |
Collaborator
There was a problem hiding this comment.
I don't remember: is import.browser.production necessary? Or can we just have import.production?
Collaborator
Author
There was a problem hiding this comment.
I tried import.production first, but the build:production script of MMS did not pick it up. Seems like it's a necessary structure to have the bundler use it.
nima-taheri-mongodb
changed the title
Contributor
|
Coverage after merging cloudp-352308-env-specific-bundles-experiment into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎫 Ticket
LG-5764
📝 Description
Added a new
modernDevProdConfigRollup configuration that generates both minified and non-minified bundles in a single pass. Limited to@lg-charts/legendas an experiment. 🧪What changed:
package.jsonexports updated withbrowser.productionentries pointing to minified bundles'inline'to separate.mapfilesWhy:
🧪 Documentation and Testing
@lg-charts/legendpackage, I confirmed locally that both non-minified and minified bundles work on MMS. Also can confirm that the minified bundle gets picked up by MMS's webpack configuration automatically.pnpm changesetand included documentation of changes