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(prerender): remove baseURL from generated file paths #329

Merged
merged 3 commits into from Aug 1, 2022

Conversation

farnabaz
Copy link
Contributor

@farnabaz farnabaz commented Jul 6, 2022

πŸ”— Linked issue

❓ 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

Generating a project using custom baseURL results unwanted directory inside .output/public.
For example, if you set baseURL = romi-project, you will see a romi-project directory in the generated website. https://github.com/hacknug/romi-project/tree/gh-pages

πŸ“ Checklist

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

@farnabaz
Copy link
Contributor Author

farnabaz commented Jul 6, 2022

Maybe I'm wrong about the behavior of Nuxt pretender. Should it generate a subdirectory inside .output/public?

What I think is that the generated website should not have a subdirectory inside it. When I set baseURL in the config, I want to deploy it under a directory in a host. Generating this subdirectory causes duplicate baseURL

Feel free to close this PR if it does not look good :)

@danielroe
Copy link
Member

danielroe commented Jul 6, 2022

Are you sure the previous PR doesn't resolve this issue? That was initially what I did @farnabaz - check the PR. But we adjusted it to remove the baseURL before the actual fetch call.

@danielroe
Copy link
Member

Ah. I think what we actually need to do here is remove the baseURL from the filePath: https://github.com/unjs/nitropack/blob/486f236c1e96134c0ee8021ff5d04e93fe819e75/src/prerender.ts#L66

@farnabaz
Copy link
Contributor Author

farnabaz commented Jul 6, 2022

Yes removing baseURL from filePath will do the trick too.

But I think the best way is to remove baseURL from routes and add it in the fetch call. This way we can support URLs like this

/baseUrl/baseUrl/something

Imagine user has a page with the same name as baseURL

@danielroe
Copy link
Member

I think that's a good point. But we will still need to update the filenames

@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #329 (44aaddd) into main (db1d087) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   55.94%   55.95%   +0.01%     
==========================================
  Files          54       54              
  Lines        3466     3467       +1     
  Branches      362      362              
==========================================
+ Hits         1939     1940       +1     
  Misses       1178     1178              
  Partials      349      349              
Impacted Files Coverage Ξ”
src/prerender.ts 66.43% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update db1d087...44aaddd. Read the comment docs.

@danielroe danielroe changed the title fix(prerender): remove baseURL from extracted links fix(prerender): remove baseURL from generated file paths Aug 1, 2022
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.

LGTM. We might add a symbolic link to avoid breaking change and allow cases when public structure is expected to have base url

@pi0 pi0 merged commit 26c15ca into main Aug 1, 2022
@pi0 pi0 deleted the fix/remove-baseurl branch August 1, 2022 15:27
WinterYukky pushed a commit to WinterYukky/nitro that referenced this pull request Nov 1, 2022
Co-authored-by: Daniel Roe <daniel@roe.dev>
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.

None yet

3 participants