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

perf: reduce bundle size for Object.keys(import.meta.glob(...)) / Object.values(import.meta.glob(...)) #18666

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

btea
Copy link
Collaborator

@btea btea commented Nov 14, 2024

Description

close #18662

@sapphi-red
Copy link
Member

I think we should optimize Object.keys(import.meta.glob / Object.values(import.meta.glob instead of introducing a new query so that users won't need to think about it.
It would be great if rollup or a minifier can optimize this case, but I guess it's easier to do it on our side.

@btea
Copy link
Collaborator Author

btea commented Nov 19, 2024

Thanks for your suggestion. But maybe we just need to optimize Object.keys(import.meta.glob, and Object.values(import.meta.glob should follow the original logic.

@btea
Copy link
Collaborator Author

btea commented Nov 20, 2024

Thanks for the review, the code has been updated.

@sapphi-red sapphi-red changed the title feat: import.meta.glob support query ?nocontent perf: reduce bundle size for Object.keys(import.meta.glob(...)) Nov 20, 2024
@sapphi-red sapphi-red added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 20, 2024
sapphi-red
sapphi-red previously approved these changes Nov 20, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

bluwy
bluwy previously approved these changes Nov 20, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

The linked issue also requests supporting Object.values, should we not close the issue until that's supported?

@btea
Copy link
Collaborator Author

btea commented Nov 20, 2024

As far as I understand, Object.values(import.meta.glob('')) can only follow the normal parsing process to get the corresponding object and then get the corresponding values. Are there any other optimization methods here?

@bluwy
Copy link
Member

bluwy commented Nov 20, 2024

I was thinking that the keys could be minimized, like "0" or "1" instead of paths like "./src/foo.png".

I was also thinking there could be a further optimization in the future where we replace the Object.keys() or Object.values() altogether and directly replace them as an array, but that's harder and we can leave that for later. So if the PR is more of a stop-gap optimization for Object.keys(), maybe we can also do a stop-gap for Object.values().

@btea
Copy link
Collaborator Author

btea commented Nov 20, 2024

@bluwy Thanks for the detailed instructions. 👍

@sapphi-red
Copy link
Member

It'd be nice to replace Object.values(import.meta.glob('')) with Object.values([() => import('')]). I'm open with having that. That said, if the minifier implements the more advanced minification of Object.values({ 'a': 'b' }) -> ['b'], we don't need that optimization. I'm fine with merging this PR without that change for that reason.

On the other hand, replacing Object.keys({ a: () => import('') }) with Object.keys({ a: '' }) has a benefit to do it on the Vite side. This will remove the unused import() and skips that file from being bundled by rollup. We cannot achieve that by improving a minifier because we run it after the chunks are generated.

@btea btea dismissed stale reviews from bluwy and sapphi-red via d869760 November 21, 2024 03:49
@btea
Copy link
Collaborator Author

btea commented Nov 21, 2024

Thank you for your guidance. I have processed Object.values(import.meta.glob('')) according to @sapphi-red suggestion.

@bluwy
Copy link
Member

bluwy commented Nov 21, 2024

Yeah I think it's worth implementing the Object.values optimization for now since it doesn't seem like any minifier implements it yet. Thanks for updating with it @btea

bluwy
bluwy previously approved these changes Nov 21, 2024
@sapphi-red sapphi-red changed the title perf: reduce bundle size for Object.keys(import.meta.glob(...)) perf: reduce bundle size for Object.keys(import.meta.glob(...)) / Object.values(import.meta.glob(...)) Nov 22, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks for updating!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize bundle for Object.keys(import.meta.glob()) and Object.values(import.meta.glob())
3 participants