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-pwa): Fix escaping in shell command create-pwa sends to buildpack #3022

Merged
merged 10 commits into from
Mar 10, 2021

Conversation

zetlen
Copy link
Contributor

@zetlen zetlen commented Feb 24, 2021

Description

  • Add escaping of space and quote characters in command path to buildpack
  • Fix Node > 14 bug in DEBUG_PROJECT_CREATION

Related Issue

Closes #2317.

Acceptance

Verification Stakeholders

@fooman

Specification

Verification Steps

Follow steps in #2317 by renaming your repo root folder something with spaces in it. Try both kinds of quotation marks as well!

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

…ildpack

Fixes #2317.

- Add escaping of space and quote characters in command path to buildpack
- Fix Node > 14 bug in DEBUG_PROJECT_CREATION
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 24, 2021

Messages
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against d15a336

@fooman
Copy link
Contributor

fooman commented Feb 25, 2021

Just to confirm the fix works on my end. Thanks @zetlen for tackling this.

@supernova-at supernova-at added the version: Patch This changeset includes backwards compatible bug fixes. label Mar 2, 2021
supernova-at
supernova-at previously approved these changes Mar 2, 2021
sirugh
sirugh previously approved these changes Mar 3, 2021
@dpatil-magento
Copy link
Contributor

@zetlen Space part looks good. But when meant quotation, should something like folder "with" space this also work?

bin/sh: /Users/sdr/Documents/PWA/folder with space/packages/pwa-buildpack/bin/buildpack: No such file or directory
(node:23411) UnhandledPromiseRejectionWarning: Error: Command failed: /bin/sh -c /Users/sdr/Documents/PWA/folder\ "with"\ space/packages/pwa-buildpack/bin/buildpack create-project fawfg --template "venia-concept" --name "fawfg" --author "tester test@test.com" --backend-url "https://master-7rqtwti-mfwmkrjfqvbjk.us-4.magentosite.cloud/" --braintree-token "sandbox_8yrzsvtm_s2bg8fs563crhqzk" --npm-client "npm"

@dpatil-magento
Copy link
Contributor

@zetlen project create part with say folder "with' space works fine but once inside scaffolded project folder "with' space/scaffold-project then commands like npm run build npm run watch npm run storybook fails with errors. I was thinking create separate bug for this as original bug folder with space works fine for all commands. Also, its very rare to have directory name with single/double quotation marks. Let me know if you are ok with this.

There is small prettier fix you need to push though :)

@dpatil-magento
Copy link
Contributor

Created https://jira.corp.magento.com/browse/PWA-1527 to handle single/double quotation marks. This PR is good to merge after prettier fix.

@dpatil-magento dpatil-magento merged commit 318f04a into develop Mar 10, 2021
@dpatil-magento dpatil-magento deleted the zetlen/fix-2317-spaces-in-scaffold-folder branch March 10, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:create-pwa pkg:venia-concept Progress: done version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: create-pwa can't handle a space in the folder hierarchy
6 participants