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: handle path with dollar signs #1732

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented Jan 26, 2021

What's the problem this PR addresses?

Vite doesn't work when the path it is in contains dollar signs

Fixes #1423
Ref https://twitter.com/youyuxi/status/1354094331876016134

How did you fix it?

Use the callback version of .replace.

More

This should be handled correctly in all places that uses .replace (for example

.replace(`__MODE__`, JSON.stringify(config.mode))
.replace(`__BASE__`, JSON.stringify(config.base))
.replace(`__ROOT__`, JSON.stringify(config.root))
.replace(`__DEFINES__`, JSON.stringify(config.define || {}))
.replace(`__HMR_PROTOCOL__`, JSON.stringify(protocol))
.replace(`__HMR_HOSTNAME__`, JSON.stringify(host))
.replace(`__HMR_PORT__`, JSON.stringify(port))
.replace(`__HMR_TIMEOUT__`, JSON.stringify(timeout))
.replace(`__HMR_ENABLE_OVERLAY__`, JSON.stringify(overlay))
) but this serves more as a pointer than a full fix in the entire codebase

@yyx990803
Copy link
Member

I really doubt this makes any difference at all. I created a project with $$ in its path, and also tried a package with $$ in its name, both are working fine (as long as no Yarn 2 is involved).

@yyx990803
Copy link
Member

The replacement value cannot be a function. It also has nothing to do with native .replace calls. I have no idea what this fix is attempting to address.

@yyx990803 yyx990803 closed this Jan 26, 2021
@arcanis
Copy link

arcanis commented Jan 26, 2021

I really doubt this makes any difference at all. I created a project with $$ in its path, and also tried a package with $$ in its name, both are working fine (as long as no Yarn 2 is involved).

I think you're incorrect ... I literally just followed your website's getting started:

yarn create @vitejs/app
cd vite-project
npm install
npm run dev

I see "Hello Vite!". I then run (note the quotes around $$):

cd ..
mv vite-project '$$vite-project'
cd '$$vite-project'
npm run dev

Navigating to localhost yields an empty page and a 404 error. Just to double-check I then do a last test:

cd ..
cp -rf '$$vite-project' foo
cd foo
npm run dev

The "Hello Vue!" appears again. In other word, the $$ caused a 404 to appear.

@arcanis
Copy link

arcanis commented Jan 26, 2021

The replacement value cannot be a function. It also has nothing to do with native .replace calls. I have no idea what this fix is attempting to address.

The replace function replaces $1, $2, etc by their equivalent in the source regex. One little known feature is that it also replaces $$ by $ (basically, it escapes $ with itself).

The diff @merceyz's nicely made aims to prevent this behavior, which causes the breakages with $$ in the path (which I assume you may have not seen if you forgot to escape '$$' in your own shell, which gets translated as the current pid). By making the replacement a function (rather than a literal), the JS engine won't try anymore to interpret whatever you return, and will instead use it at face value - which is what you want.

Honestly, even if you don't repro the problem (and I think you should be able to fairly easily), it's easy to see what kind of problems the current code is susceptible to.

@merceyz
Copy link
Contributor Author

merceyz commented Jan 26, 2021

The replacement value cannot be a function

It's passed into https://github.com/rollup/plugins/blob/f75d5a7fff89bff5f3d59cb268926337b709598a/packages/alias/src/index.ts#L106 and per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#parameters it can be a function, if it goes anywhere else I missed please, do tell

@yyx990803
Copy link
Member

Ah, ok, this only happens if vite's own path includes $$. I've been testing with locally linked vite which isn't affected by the project path.

@yyx990803 yyx990803 reopened this Jan 26, 2021
@yyx990803 yyx990803 merged commit 20bacf7 into vitejs:main Jan 26, 2021
@yyx990803
Copy link
Member

Thanks for the fix! The windows CI is failing because of unrelated commits.

@merceyz
Copy link
Contributor Author

merceyz commented Jan 26, 2021

Ah, ok, this only happens if vite's own path includes $$.

That's what I said in the PR description (and why I applied the change to the CLIENT_DIR), but it can happen in any of the .replace calls that don't escape the replacement

Thanks for the fix!

No worries, if you run into issues concerning Yarn PnP please don't hessitate to ping us 👍

@merceyz merceyz deleted the merceyz/alias branch January 26, 2021 19:14
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.

/@vite/client won't load after adding peerDependencies under Yarn 2/PnP
3 participants