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

Update Vite to v6, full minify esDynamic #2490

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

milespetrov
Copy link
Member

@milespetrov milespetrov commented Nov 26, 2024

Changes

  • update vite dependency from v5 -> v6
  • move npm build output into its own folder dist/bundler
  • add custom vite plugin to force full minification on esDynamic
  • vite config refactor to simplify things and take advantage of the new cssFileName option in vite v6
  • removed ts error in fixtures/settings/templates/input-control.vue showing up on GitHub

Notes

vite doesn't yet handle the case for es builds in library mode for native only imports. It assumes there will always be a bundler so it leaves the output partially unminified (things like pure annotations that are required by bundlers). Since esDynamic is meant for native in browser imports only its safe to force a full minification. More info: vitejs/vite#6555

Size tally (initial download, gzip)

main branch: 1850 kB
PR #2489: 1523 kB (dynamic imports)
This PR: 1309 kB (full minification, including above pr)

QA Testing

Please test against main branch https://ramp4-pcar4.github.io/ramp4-pcar4/main/index-esm.html

Testing

  1. Open https://milespetrov.github.io/ramp4-pcar4/vite-6/index-esm.html
  2. General sanity testing
  3. Open network tab and compare js download sizes to the main branch

This change is Reviewable

@milespetrov milespetrov added the PR: Build PR that involves changes to the build. This includes changes to the Github Actions. For JS wizards. label Nov 26, 2024
@milespetrov milespetrov changed the title Update Vite to v6, remove esDynamic comments Update Vite to v6, full minify esDynamic Nov 29, 2024
Copy link
Member

@james-rae james-rae left a comment

Choose a reason for hiding this comment

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

Just curious, whats the difference between folders bundler and esDynamic. They both look chunky build outputs but are not identical.

Reviewed 4 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @milespetrov)


src/fixtures/settings/templates/input-control.vue line 24 at r2 (raw file):

<script setup lang="ts">
import config from '@arcgis/core/config';

lol good catch. I'm assuming someone did a mistake auto-import when typing the props. And the Vue binding to the prop fools VSCode into thinking it's actively used.

Copy link
Member Author

@milespetrov milespetrov left a comment

Choose a reason for hiding this comment

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

bundler files don't include any ramp dependencies and are not fully minified. The consuming project will npm grab our deps and their bundler will smash things together as it pleases. esDynamic has all dependencies included and is fully minified. It is used as is, no npm or build steps required.

So if you have a project that has shared dependencies with ramp, say vue and ag-grid, then npm installing ramp (bundler files) is best because both ramp and your project code will share one copy of shared deps. With esDynamic ramp will always have its own deps and your project will have another copy.

Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @james-rae)

Copy link
Member

@james-rae james-rae left a comment

Choose a reason for hiding this comment

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

Great, thanks. And I'mma assume this line in pacakge.json is what makes npm smart to this and "it just works"

"module": "./dist/bundler/ramp.bundle.es.js"

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: 6 of 7 files reviewed, all discussions resolved (waiting on @milespetrov)

Copy link
Member

@james-rae james-rae left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @milespetrov)

Copy link
Member Author

@milespetrov milespetrov left a comment

Choose a reason for hiding this comment

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

You got it

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @milespetrov)

Copy link
Member

@szczz szczz left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, 2 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @milespetrov)

@james-rae james-rae merged commit 1fb1a90 into ramp4-pcar4:main Dec 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Build PR that involves changes to the build. This includes changes to the Github Actions. For JS wizards.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants