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

shell-quote windows: unescape specific prefix char (@) #4252

Closed
wants to merge 1 commit into from
Closed

shell-quote windows: unescape specific prefix char (@) #4252

wants to merge 1 commit into from

Conversation

codenoid
Copy link

What this PR solves / how to test:

Related to #4105

Some package installation that begin with @ (ex: @sveltejs/adapter-cloudflare package) will break because the output of current shell-quote are \@sveltejs/adapter-cloudflare (instead of @sveltejs/adapter-cloudflare) which the beginning \ will be read as C:\ instead of escaping the @ char

My PR here to bring temporary fixes for the case

Another thing I found is that pnpm will treat \@sveltejs/adapter-cloudflare as @sveltejs/adapter-cloudflare while npm read it as a local path

Log with 3.14.0

╰─ff wrangler init sveltetest
 ⛅️ wrangler 3.14.0
-------------------
Using npm as package manager.
▲ [WARNING] The `init` command is no longer supported. Please use `npm create cloudflare@2 -- sveltetest` instead.

  The `init` command will be removed in a future version.


Running `npm create cloudflare@2 -- sveltetest`...

using create-cloudflare version 2.6.1

╭ Create an application with Cloudflare Step 1 of 3
│
├ In which directory do you want to create your application?
│ dir ./sveltetest
│
├ What type of application do you want to create?
│ type Website or web app
│
├ Which development framework do you want to use?
│ framework Svelte
│
╰ Continue with Svelte via `npx create-svelte@5.1.1 sveltetest`


create-svelte version 5.1.1

┌  Welcome to SvelteKit!
│
◇  Which Svelte app template?
│  SvelteKit demo app
│
◇  Add type checking with TypeScript?
│  Yes, using JavaScript with JSDoc comments
│
◇  Select additional options (use arrow keys/space bar)
│  none
│
└  Your project is ready!

✔ Type-checked JavaScript
  https://www.typescriptlang.org/tsconfig#checkJs

Install community-maintained integrations:
  https://github.com/svelte-add/svelte-add

Next steps:
  1: cd sveltetest
  2: npm install
  3: git init && git add -A && git commit -m "Initial commit" (optional)
  4: npm run dev -- --open

To close the dev server, hit Ctrl-C

Stuck? Visit us at https://svelte.dev/chat

╭ Configuring your application for Cloudflare Step 2 of 3
│
├ Installing dependencies
│ installed via `npm install`
│
├ Adding the Cloudflare Pages adapter
│ npm ERR! code ENOENT
npm ERR! syscall open
npm ERR! path C:\@sveltejs\adapter-cloudflare/package.json
npm ERR! errno -4058
npm ERR! enoent ENOENT: no such file or directory, open 'C:\@sveltejs\adapter-cloudflare\package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent

npm ERR! A complete log of this run can be found in: C:\Users\XXX\AppData\Local\npm-cache\_logs\2023-10-21T00_43_25_312
Z-debug-0.log

│
Error: npm ERR! code ENOENT
npm ERR! syscall open
npm ERR! path C:\@sveltejs\adapter-cloudflare/package.json
npm ERR! errno -4058
npm ERR! enoent ENOENT: no such file or directory, open 'C:\@sveltejs\adapter-cloudflare\package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent

npm ERR! A complete log of this run can be found in: C:\Users\Rubi\AppData\Local\npm-cache\_logs\2023-10-21T00_43_25_312Z-debug-0.log

npm ERR! code 1
npm ERR! path C:\Users\XXX\Documents\sat\undian-online\shellquotetest
npm ERR! command failed
npm ERR! command C:\WINDOWS\system32\cmd.exe /d /s /c create-cloudflare sveltetest

npm ERR! A complete log of this run can be found in: C:\Users\XXX\AppData\Local\npm-cache\_logs\2023-10-21T00_42_50_006Z-debug-0.log

✘ [ERROR] Command failed with exit code 1: npm create cloudflare@2 -- sveltetest

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@codenoid codenoid requested a review from a team as a code owner October 21, 2023 00:44
@changeset-bot
Copy link

changeset-bot bot commented Oct 21, 2023

⚠️ No Changeset found

Latest commit: b43888d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6594126417/npm-package-wrangler-4252

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6594126417/npm-package-wrangler-4252

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6594126417/npm-package-wrangler-4252 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6594126417/npm-package-cloudflare-pages-shared-4252

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.14.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare 3.20231016.0 3.20231016.0
workerd 1.20231016.0 1.20231016.0
workerd --version 1.20231016.0 2023-10-16

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@codenoid codenoid changed the title shell-quote windows: unescape specific char (@) shell-quote windows: unescape specific prefix char (@) Oct 21, 2023
@mrbbot
Copy link
Contributor

mrbbot commented Nov 13, 2023

Hey! 👋 Great catch, and thanks for submitting this PR! Would you be able to add a test and a changeset for this behaviour? It would also be good to rebase this as we've landed some changes recently to improve the reliability of our current tests. 🙂

@mrbbot mrbbot added the awaiting reporter response Needs clarification or followup from OP label Nov 13, 2023
@codenoid
Copy link
Author

no

@dario-piotrowicz
Copy link
Member

I'm pretty sure this has already been fixed in #4445

But thanks so much for the PR anyways @codenoid 🙏

@codenoid codenoid deleted the shellquote branch November 25, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reporter response Needs clarification or followup from OP
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants