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(create-vite): fix broken vite logo when base changed #12374

Merged
merged 2 commits into from
Mar 12, 2023

Conversation

XanderXAJ
Copy link
Contributor

@XanderXAJ XanderXAJ commented Mar 11, 2023

Description

After using create-vite to generate a React project, in a production build the Vite logo will break when setting the base URL to anything except the default /. This came up for me when trying to build GitHub Pages in a repo of mine, which requires the base to match the repo name.

For example, this build process will result in a broken Vite logo:

# To replicate the issue
npm run build -- --base /test
npm run preview -- --base /test

This occurs because the img src directly refers to the generated /vite.svg and React does not preprocess src attributes to add the configured base.

As suggested in #7358 (found via #10601), the solution is to import vite.svg in the same way as react.svg already is. This MR implements that solution in to the templates to prevent confusion in the future.

Note: This may also apply to the preact templates. However, I've not modified them as I'm not currently familiar with preact.

Without the fix, using a modified base:

image

With the fix, using a modified base:

image

Additional context

If using npm run dev from the generated package.json, the issue will not show up. However, the issue will show up when using npm run build followed by npm run preview.

# This will replicate the issue
npm run build -- --base /test
npm run preview -- --base /test

# This will NOT replicate the issue
npm run dev -- --base /test 

I've tested this by modifying a generated project to verify the results of a fix. However, I've not tested this using npm create vite... as I'm unsure how to perform a representative test via that method.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

After using create-vite to generate a React project, in a production build the Vite logo will break when setting the base URL to anything except the default `/`.

This occurs because the `img` `src` directly refers to the generated `/vite.svg` and React does not preprocess `src` attributes to add the configured `base`.

As suggested in vitejs#7358 (found via vitejs#10601), the solution is to import `vite.svg` in the same way as `react.svg` already is. This MR implements that solution in to the templates to prevent confusion in the future.

Note: If using `npm run dev` from the generated `package.json`, the issue will not show up. However, the issue will show up when using `npm run build` followed by `npm run preview`.

```shell
npm run build -- --base /test
npm run preview -- --base /test

npm run dev -- --base /test
```
@stackblitz
Copy link

stackblitz bot commented Mar 11, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

patak-dev
patak-dev previously approved these changes Mar 11, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Thanks!

@bluwy
Copy link
Member

bluwy commented Mar 11, 2023

This seems to affect all templates (except Vue because it auto imports it), maybe we should update them all at once.

@XanderXAJ
Copy link
Contributor Author

I'll take a look at the other templates @bluwy. 👍

It's been a few years since I last did any serious front-end dev (my experience is in backend DevEx/Platforms engineering) and I only started trying out Vite today on a whim, but this is a good excuse to brush up. I'll let you know if I have any issues.

I'll mark this as draft in the meantime.

@XanderXAJ XanderXAJ marked this pull request as draft March 11, 2023 17:21
@XanderXAJ
Copy link
Contributor Author

XanderXAJ commented Mar 11, 2023

I've pushed fixes for all of the other templates that needed them. I've added further info below in case it helps anyone.

Since I copied the changed files from my generated test projects back to the templates, there are a couple of whitespace differences. I've left them in since that means that the templates will now be closer to the files they generate. I'm happy to remove the whitespace changes if they're not desired.

Yep, they're all broken (except Vue)

Sure enough, I can confirm it is broken in the other templates:
image
image
image
image

(Lit doesn't have a preview as it generates only a component -- I checked the generated source code for the component manually.)

For completeness, I checked Vue as well -- as expected, it works without further modification (both JS and TS variants):
image

Minimising human error

I cooked up a script to create and run all of the different templated projects one by one:

#!/usr/bin/env bash
trap '' INT # Allow interrupting `npm run preview` servers with Ctrl-C without terminating the whole loop

echo No Base
for template in lit lit-ts preact preact-ts svelte svelte-ts vanilla vanilla-ts vue vue-ts; do
	echo "$template"
	if [ ! -d "$template" ]; then
		echo Creating project for ${template}...
		npm create vite@latest "$template" -- --template "$template"
		( cd "$template" && npm install )
	fi

	echo "${template}: Default base"
	( cd "$template" && npm run build )
	# lit only generates a component, so it has no preview
	if [[ "$template" != lit* ]]; then
		( cd "$template" && npm run preview -- --open )
	fi

	echo "${template}: Configured base"
	( cd "$template" && npm run build -- --base /test )
	# lit only generates a component, so it has no preview
	if [[ "$template" != lit* ]]; then
		( cd "$template" && npm run preview -- --open --base /test )
	fi
done

trap INT # Reset interrupt capture back to default

The script uses create-vite to generate a project using each template and installs its dependencies. It then builds and previews each project twice: once with the default base, and once with a configured /test base.

I ran this script twice. Once without any fixes. Then once more after I applied the fixes. This made it very easy to see which projects worked without changes (Vue) and which required fixes (the rest).

To minimise the chance for mistakes, since I don't know a way to generate a new project from an updated template that hasn't been merged upstream, I instead copied each fixed file from the generated project back to the template. For example:

file=vanilla-ts/src/main.ts ; cp $file ../vite/packages/create-vite/template-$file

As noted above, this resulted in minor whitespace changes in two cases, svelte and svelte-ts, which I've left in as that brings the templates closer to the files they generate.

@XanderXAJ XanderXAJ marked this pull request as ready for review March 11, 2023 18:50
@XanderXAJ
Copy link
Contributor Author

Re-opened for review.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for taking the time to check all of them. It is good you didn't modify the Vue template, as these URLs in the SFC templates are processed as you discovered 👍🏼

@XanderXAJ XanderXAJ changed the title fix(create-vite): fix broken vite logo in react when base changed fix(create-vite): fix broken vite logo when base changed Mar 11, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Great changes! Thanks for fixing the rest of the templates.

@bluwy bluwy merged commit 2b472d1 into vitejs:main Mar 12, 2023
XanderXAJ added a commit to XanderXAJ/vite that referenced this pull request Mar 12, 2023
As part of my changes in vitejs#12374, I unintentionally removed the quotes
for the Vite logos from the vanilla templates, perhaps because the rest
of the templates didn't need them.

While HTML5 allows no quotes for attribute values if the value doesn't contain
a space, keeping a consistent style is important for cognitive load when
reading and maintaining. Hence, I've marked this as a style change.

While not a functional change in these templates, it could be if someone
later changed the import path to contain a space (perhaps after
generating a project using the template).
@XanderXAJ XanderXAJ deleted the XanderXAJ/fix-create-react-image-base branch December 11, 2023 13:01
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.

3 participants