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: ignore dollar replacement sequences in file paths #3662

Closed
wants to merge 1 commit into from

Conversation

charlessuh
Copy link
Contributor

When using string.replace as a templating engine, care must be taken to escape dollar signs as they can be interpreted as replacement patterns: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#specifying_a_string_as_a_parameter

Note yarn uses a special $$virtual folder in some circumstances, so this can break setups involving yarn.

@google-cla
Copy link

google-cla bot commented Mar 7, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@charlessuh
Copy link
Contributor Author

@googlebot I signed it!

@AppVeyorBot
Copy link

Build karma 2944 completed (commit 2046e54eb6 by @charlessuh)

@karmarunnerbot
Copy link
Member

Build karma 547 completed (commit 2046e54eb6 by @charlessuh)

@karmarunnerbot
Copy link
Member

Build karma 546 completed (commit 2046e54eb6 by @charlessuh)

@devoto13
Copy link
Collaborator

devoto13 commented Mar 8, 2021

Thanks for a PR @charlessuh! However, I think the approach from #3531 is better as it targets the root cause of the problem and does not create an implicit dependency between the generation of URLs and how we format them into the page body.

If you're interested in this fix, maybe you can build on top of that PR and fix the comment there, so we can get it in?

@charlessuh
Copy link
Contributor Author

Ah, awesome.

@charlessuh charlessuh closed this Mar 8, 2021
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.

4 participants