Skip to content

fix: remove effect packages from optimizePackageImports list #80761

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

Closed
wants to merge 1 commit into from

Conversation

fubhy
Copy link

@fubhy fubhy commented Jun 22, 2025

The feature for optimizeModuleImports seems to originate here

The effect packages were first introduced here.

The original intent seems to have been to optimise performance during development time. But it has questionable side effects outside of this intent.

The way aliases are generated here is problematic for at least two reasons:

a) multiple versions of the same package now have their root entrypoint aliased to the same package without taking proper module resolution into consideration
b) it only targets exact matches on root entrypoints - the module exports of these packages are still resolved with the normal module resolution algorithm

This combination means that we get completely undefined runtime behavior.

This PR removes the effect packages from this list to circumvent this problem as a last resort, but I'd suggest to fix this behavior more sustainably instead(!) as it has the potential to generate incredibly difficult to debug runtime behavior that differs between development & production. It makes it easy to ship application versions into production that are broken in ways that are not surfaced during development.

The good intention to cirumvent the the module traversal and scanning performance impact in case of barrel file imports during development time is noble but it should not have the unintended side effects with these server path alias rewrites as observed.

NOTE: The negative impact of this behavior goes beyond effect and would equally apply to some of the other packages in that list with potentially worse outcomes. Instead of explicitly breaking on load it would cause undefined runtime behavior for most other packages.

Copy link

vercel bot commented Jun 22, 2025

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding, @huozhi:

packages/next/src/server/config.ts

@ijjk
Copy link
Member

ijjk commented Jun 22, 2025

Allow CI Workflow Run

  • approve CI run for commit: c7e35c0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@fubhy
Copy link
Author

fubhy commented Jun 22, 2025

Addressed properly in #80769

@fubhy fubhy closed this Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants