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: add production export condition to commands #605

Closed
wants to merge 1 commit into from

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

resolves nuxt/nuxt#15215

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

As we trace dependencies via the production export condition, if users do not run their node output with that condition, it may cause issues for them. We may add the condition to our documentation and preview commands to prevent these issues (and possibly overall increase performance).

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added bug Something isn't working enhancement New feature or request labels Oct 20, 2022
@danielroe danielroe requested a review from pi0 October 20, 2022 21:17
@danielroe danielroe self-assigned this Oct 20, 2022
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #605 (6ec410c) into main (400c176) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     nuxt/bridge#212   +/-   ##
=======================================
  Coverage   66.10%   66.10%           
=======================================
  Files          55       55           
  Lines        4116     4116           
  Branches      445      445           
=======================================
  Hits         2721     2721           
  Misses       1391     1391           
  Partials        4        4           
Impacted Files Coverage Ξ”
src/presets/node-cli.ts 100.00% <100.00%> (ΓΈ)
src/presets/node-server.ts 100.00% <100.00%> (ΓΈ)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pi0
Copy link
Member

pi0 commented Oct 21, 2022

This really only solves the manual/preview commands. Production deployments rely on server.mjs are still affected.

@danielroe
Copy link
Member Author

Production deployments should be launched with the condition as well.

@pi0
Copy link
Member

pi0 commented Oct 21, 2022

Is there an argument or option for example for vercel to do it?

Copy link
Member Author

I'm not sure. I'll follow up with them directly. But I would guess that setting a NODE_OPTIONS environment variable would allow it to be done.

Copy link
Member Author

... and I believe we can do that from the FS API. Will check. Shall I add to this PR or make a separate one?

@pi0
Copy link
Member

pi0 commented Oct 21, 2022

Indeed we may find solutions per (some) providers too. My point is, probably right solution instead of requiring the condition is to rewrite written package.json exports to make production default. We are already doing copy it is not that hard and is a universal fix.

@danielroe
Copy link
Member Author

Sounds like a good idea πŸ‘

@pi0
Copy link
Member

pi0 commented Jan 16, 2023

I think we can rework this with new externals plugin and fine-gained access to package.json writing. #844 (feel free to pick it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After build not all library files are copied to .output dir
2 participants