Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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