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 for hoisted scripts in project with spaces in the file path #5756

Merged
merged 3 commits into from
Jan 4, 2023

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Jan 4, 2023

Note that this will be backported to 1.0 after being cherry-picked.

Changes

Testing

  • Test added

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2023

🦋 Changeset detected

Latest commit: c6ddd87

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 Jan 4, 2023
Comment on lines 52 to 53
const url = new URL('.' + filename, config.root);
filename = prependForwardSlash(fileURLToPath(url));
Copy link
Member

Choose a reason for hiding this comment

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

Will this work on Windows? I assume pathname was being used here to ensure that we were getting the normalized /path/to/file but fileURLToPath(url) will give a platform-dependent file path. Maybe the result of that needs to be normalized before the forward slash is prepended?

Copy link
Member

Choose a reason for hiding this comment

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

If Windows tests pass, then I digress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah there's something wrong here. We should be using fileURLToPath though, otherwise you get %20 for the spaces. Something else is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I think prependForwardSlash(normalizePath(fileURLToPath(url))) would be correct? Using normalizePath from Vite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

viteID did it. the problem was that fileURLToPath uses back slashes. viteID fixes it.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great! Insert [space] pun here.

@matthewp matthewp merged commit 116d883 into main Jan 4, 2023
@matthewp matthewp deleted the spaces branch January 4, 2023 20:36
matthewp added a commit that referenced this pull request Jan 4, 2023
* Fix for hoisted scripts in project with spaces in the file path

* Adding a changeset

* Use viteID instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro’s dev server can't bundle when there is a space in a parent folder
2 participants