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: use path for renderer entrypoint #12563

Closed
wants to merge 3 commits into from
Closed

fix: use path for renderer entrypoint #12563

wants to merge 3 commits into from

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Nov 29, 2024

Changes

#12533 changed the MDX entrypoint to a URL, so that it could be resolved correctly. However this introduced a different bug that meant the entrypoint could not be resolved in dev if there was a space in the path. It's unclear whether this is a Vite bug or ours, but we can fix it by using fileURLToPath instead of passing the URL directly. This PR converts any entrypoint URLs to paths.

Fixes #12556

Testing

This is hard to test in the monorepo, because it needs a space in the path to the integration not the site. I tested it by renaming my monorepo folder, but I couldn't work out an easy way to do a test fixture for it.

Docs

Copy link

changeset-bot bot commented Nov 29, 2024

🦋 Changeset detected

Latest commit: 122bd67

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 29, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I remember we had a similar issue when loading a middleware file inside a project that had a space in its name.

Since both files use the vite module loader in dev, I believe it's a vite issue.

Happy with the fix

Copy link

codspeed-hq bot commented Nov 29, 2024

CodSpeed Performance Report

Merging #12563 will degrade performances by 16%

Comparing mdx-path (c2d800e) with main (c50882d)

Summary

⚡ 1 improvements
❌ 2 regressions
✅ 3 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main mdx-path Change
Rendering: streaming [false], .astro file 1.5 s 1.8 s -14.64%
Rendering: streaming [false], .md file 22.4 ms 15 ms +49.46%
Rendering: streaming [false], .mdx file 1 s 1.2 s -16%

@ascorbic
Copy link
Contributor Author

I'll see if I can do a pure-Vite repro

@ascorbic
Copy link
Contributor Author

Ah damn this is failing on Windows now. Weird because fileURLToPath should be robust there. I think we may need another approach.

@ascorbic
Copy link
Contributor Author

ascorbic commented Nov 29, 2024

Yup, it's upstream. I think we should close this rather than trying to work around it. I'll try another approach for the MDX case specifically. vitejs/vite#9917 @bluwy

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) and removed pkg: astro Related to the core `astro` package (scope) labels Nov 29, 2024
@ascorbic ascorbic closed this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't build astro if project path contains spaces on MacOS
2 participants