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: Remove side effect to fix pnpm + vite issue #111

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

haines
Copy link
Contributor

@haines haines commented Feb 12, 2025

Fixes #110

This PR delays the feature detection of require.resolve until a Hook is created, which avoids an issue where Vite can bundle the module into an ESM context where require is unavailable (vitejs/vite#19403).

@timfish
Copy link
Contributor

timfish commented Feb 17, 2025

Have you tested if this actually fixes the issue with Vite?

@haines
Copy link
Contributor Author

haines commented Feb 17, 2025

@timfish yep, I applied this change to our application using pnpm patch and it fixed the issue.

@timfish timfish changed the title fix: Remove side effect on module load fix: Remove side effect to fix pnpm + vite issue Feb 17, 2025
Copy link
Contributor

@jsumners-nr jsumners-nr left a comment

Choose a reason for hiding this comment

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

I don't understand the issue. Why do we need to define a new method on Hook? What is wrong with defining the module local resolve function?

@timfish
Copy link
Contributor

timfish commented Feb 18, 2025

I don't understand the issue.

v7.5.0 added some side-effects and now when using pnpm, vite outputs invalid code. Our existing code is perfectly valid and frankly this really is not our problem, but it's impacting a lot of users and this is a simple change.

Why do we need to define a new method on Hook? What is wrong with defining the module local resolve function?

Yep, that would be better!

@timfish
Copy link
Contributor

timfish commented Feb 18, 2025

@haines can you make these changes to your PR?

I did try to push to your branch but didn't have permissions.

@haines haines force-pushed the remove-side-effect branch from 761f19f to 121ed3e Compare February 19, 2025 10:53
@haines
Copy link
Contributor Author

haines commented Feb 19, 2025

@timfish I've refactored it to keep resolve as a module-level function, but do the feature detection lazily to remove the side effect. What do you think?

@haines haines requested a review from timfish February 19, 2025 11:48
@timfish timfish merged commit 8039296 into nodejs:main Feb 21, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading from v7.4 to v7.5 breaks ESM application
3 participants