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

Port cra.sh development task to javascript #2309

Merged
merged 2 commits into from
Jan 9, 2018

Conversation

ianschmitz
Copy link
Contributor

I've ported tasks/cra.sh to javascript to make it easier for contributors on Windows based systems to run an end to end create-react-app CLI workflow as discussed in #2280.

If someone more familiar with proper handling of child processes and signal events could take a close look that would be much appreciated!

I've tested on Windows 10 using both npm and yarn in Command Prompt, PowerShell and Git Bash.

@cr101
Copy link
Contributor

cr101 commented May 23, 2017

@ianschmitz Thank you for this PR. A much-needed improvement for the Windows users.
I tested your PR on my local machine and it didn't work with Yarn 0.22.0, it gave the error below:

error An unexpected error occurred: "Invalid URI "c:/wf/create-react-app/packages/react-scripts/react-scripts-1.0.5.tgz"".

However, after I upgraded Yarn to the current stable version 0.24.5 it then worked fine.

@ianschmitz
Copy link
Contributor Author

Yes I found that same behavior as well @cr101.

I was running yarn 0.23.4. I mentioned it in the code review. Latest stable fixed it for me

@cr101
Copy link
Contributor

cr101 commented Jun 28, 2017

@gaearon I've been using this change on my local Windows 10 dev machine for a couple of months now and it works great. Can this be merged please?

@ianschmitz
Copy link
Contributor Author

The part I was hoping to get a review on was if the SIGINT event was sufficient to handle all cases such as cancelling part way through. I have only tested this on Windows.

@ianschmitz
Copy link
Contributor Author

Seems like this has been used for a while now with good success. Can we get this in soonish? :)

@xjlim
Copy link
Contributor

xjlim commented Nov 7, 2017

@Timer this is the PR you mentioned. Can confirm this works 👍
@ianschmitz it might be better to print just the message and not the stack trace.
image and the new file needs to be relicensed to MIT.

@ianschmitz
Copy link
Contributor Author

I just rebased and updated the license comment. Thoughts on the error output? @Timer?

@Timer
Copy link
Contributor

Timer commented Nov 8, 2017

I think the error is fine; this is for local devs so the more information the better. An end user will never see it.

@gaearon gaearon merged commit b20b96a into facebook:master Jan 9, 2018
@gaearon gaearon mentioned this pull request Jan 15, 2018
Pavek pushed a commit to Pavek/create-react-app that referenced this pull request Jul 10, 2018
* Port cra.sh development task to javascript

* Port cra.sh development task to javascript

Use absolute path when generating .tgz path
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants