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

fix: make --force work for all environments #18901

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

Conversation

sapphi-red
Copy link
Member

Description

--force only applied to the client environment. I think passing it should apply to other environments too.
Probably the behavior change was caused by #18465

@sapphi-red sapphi-red added has workaround p2-edge-case Bug, but has workaround or limited in scope (priority) feat: deps optimizer Esbuild Dependencies Optimization labels Dec 6, 2024
@bluwy
Copy link
Member

bluwy commented Dec 6, 2024

IIUC we wanted the top-level optimizeDeps to apply for the client environment only, so it makes sense and consistent I think for optimizeDeps.force to only apply to the client environment.

That means we likely have to alter how --force is passed to the inline config:

optimizeDeps: { force: options.force },

And it's a bit tricky to do so there since we don't know all environments upfront. We might need to inject a plugin with the configEnvironment hook to set optimizeDeps.force for all environments?

@sapphi-red
Copy link
Member Author

I added a new forceOptimization option in InlineConfig so that optimizeDeps.force only applies to the client env.
I guess we should deprecate optimizeDeps.force. It doesn't make sense to set it in the config file.

patak-dev
patak-dev previously approved these changes Dec 10, 2024
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I also think we should deprecate the force options 👍🏼

@sapphi-red sapphi-red requested a review from bluwy December 16, 2024 11:41
bluwy
bluwy previously approved these changes Dec 16, 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.

I think this makes sense 👍 I wonder if the name can be shorten as forceOptimize though, or maybe it could be more specific like forceOptimizeDeps.

hi-ogawa
hi-ogawa previously approved these changes Dec 17, 2024
@sapphi-red sapphi-red dismissed stale reviews from hi-ogawa, bluwy, and patak-dev via b418978 December 17, 2024 02:44
@sapphi-red
Copy link
Member Author

I wonder if the name can be shorten as forceOptimize though, or maybe it could be more specific like forceOptimizeDeps.

Renamed to forceOptimizeDeps. Now it looks cleaner. 🙂

@patak-dev patak-dev added this to the 6.1 milestone Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization has workaround p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants