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: pass custom entry filename when resolving prerenderer #2461

Merged
merged 5 commits into from
May 23, 2024

Conversation

brandonroberts
Copy link
Contributor

πŸ”— Linked issue

Closes #2444

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 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)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This allows the rollupConfig.options.output.entryFileNames to be passed to the nitro prerenderer if its a string, with the fallback to index.mjs. This fixes an issue where prerendering fails if a custom entry filename is used.

πŸ“ Checklist

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

src/prerender.ts Outdated Show resolved Hide resolved
Co-authored-by: Daniel Roe <daniel@roe.dev>
@brandonroberts
Copy link
Contributor Author

I'll check further into the failing tests

@brandonroberts
Copy link
Contributor Author

@danielroe I took a different approach and just override the entry filename explicitly because the prerenderer is being built internally. Let me know if you have another recommended approach.

@pi0
Copy link
Member

pi0 commented May 22, 2024

Nice find! I think we can move the default rollup config to the pretender preset.

@brandonroberts brandonroberts force-pushed the fix-prerender-server-filename branch 2 times, most recently from 7394e53 to 0033838 Compare May 22, 2024 21:06
@brandonroberts
Copy link
Contributor Author

Nice find! I think we can move the default rollup config to the pretender preset.

Cool, done!

@pi0
Copy link
Member

pi0 commented May 22, 2024

@brandonroberts sorry my bad just tried locally moving to preset won't work because explicit config overrides entry name still.

I took a different approach and just override the entry filename explicitly because the prerenderer is being built internally. Let me know if you have another recommended approach.

May i ask there was a reason to not use original approach? While since now we have only one chunk entry for pretender, if in the future we opt pretender bundle into multi chunks for less mem footprint, we might need to change solution (and since no regression test might forget). If original approach was working well, i would suggest to use that and dynamically compute serverFilename

@brandonroberts
Copy link
Contributor Author

That approach broke 3 test suites tests where the preset is already overriding the entryFileNames property in combination with custom serverDir and output options.

I'll take another look

@pi0
Copy link
Member

pi0 commented May 22, 2024

Thank you! Please let me know if could help on investing failed tests.

@brandonroberts
Copy link
Contributor Author

Should be sorted out now. I needed to use the produced nitrorenderer config to build the server filename dynamically.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

πŸ’š

@pi0 pi0 merged commit 9bd6622 into nitrojs:main May 23, 2024
4 checks passed
@brandonroberts
Copy link
Contributor Author

Thanks @pi0 and @danielroe!

@brandonroberts brandonroberts deleted the fix-prerender-server-filename branch May 23, 2024 13:14
@pi0 pi0 mentioned this pull request Jun 13, 2024
@pi0 pi0 mentioned this pull request Jun 27, 2024
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.

Prerendering fails when custom serverDir and rollup.output.entryFileNames is used
3 participants