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

Vite: Fix react-vite and projects with absolute path preview entries on Windows #21545

Merged

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Mar 10, 2023

Closes N/A

What I did

Fix vite-react-sandbox development on Windows machines

How to test

  1. Run a vite-react sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts on a Windows machine. Storybook should start up properly.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@valentinpalkovic valentinpalkovic changed the title Valentin/fix vite sandbox development on windows Fix vite sandbox development on windows Mar 10, 2023
@valentinpalkovic valentinpalkovic changed the base branch from next to valentin/fix-build-command-on-windows March 10, 2023 15:16
@valentinpalkovic valentinpalkovic requested a review from IanVS March 10, 2023 15:16
Base automatically changed from valentin/fix-build-command-on-windows to next March 10, 2023 15:19
@valentinpalkovic valentinpalkovic self-assigned this Mar 10, 2023
import { normalizePath } from 'vite';
import slash from 'slash';

function normalizePath(id: string) {
Copy link
Member

Choose a reason for hiding this comment

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

This is copied largely from the normalizePath from vite, which was used previously in builder-vite.

? {
// TODO: Evaluate if searching for node_modules in a yarn pnp environment is correct
bare: previewFile.includes('node_modules')
? stripAbsNodeModulesPath(previewFile)
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that this is necessary, but it is caused by #21197 (comment) in all projects but also could happen if the user specifies addons in their main.js using absolute paths (for yarn pnp, for instance).

I would love to find a way to avoid so many absolute paths, but it seems like for now at least it's what we are stuck with.

Note that windows will likely still break if the user specifies absolute paths to non-node_modules files, but I don't think that should not be supported anyway (I can't think of a reason it would be needed, at least).

Copy link
Member

Choose a reason for hiding this comment

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

In a pnp situation, the path won't include node_modules.

@IanVS IanVS requested review from ndelangen and tmeasday March 10, 2023 15:23
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-vite-sandbox-development-on-windows branch from 7c38ee5 to c939bbb Compare March 10, 2023 15:27
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-vite-sandbox-development-on-windows branch from c939bbb to 267df02 Compare March 13, 2023 08:22
@IanVS
Copy link
Member

IanVS commented Mar 14, 2023

I don't think this is a bug fix only for vite sandboxes, but rather any vite react project in Windows. I would love to get it fixed ASAP. @ndelangen do you have a suggestion for how to handle yarn pnp? If not, I'd like to move forward with this and fix pnp as a second step, since that is a much smaller set of users affected.

(side note: I tried for quite a while to get the yarn pnp sandbox to work, so I could experiment, but was not successful)

@IanVS IanVS changed the title Fix vite sandbox development on windows Vite: Fix react-vite and projects with absolute path preview entries on Windows Mar 14, 2023
@valentinpalkovic valentinpalkovic merged commit b5c3281 into next Mar 15, 2023
@valentinpalkovic valentinpalkovic deleted the valentin/fix-vite-sandbox-development-on-windows branch March 15, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants