Add global deprecate warning on create-react-app package #7452
Conversation
sagarnasit
requested review from
bugzpodder,
ianschmitz,
iansu,
mrmckeb and
petetnt
as code owners
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
@mrmckeb I have created basic PR here. Tried this solution you provided but it seems it is not working well. Let me know how to proceed next. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -124,6 +125,10 @@ const program = new commander.Command(packageJson.name) | |||
| }) | |||
| .parse(process.argv); | |||
|
|
|||
| if (isInstalledGlobally) { | |||
| console.log(`global installation for create-react-app is deprecated.`); | |||
There was a problem hiding this comment.
I think is good if you use console.warn with chalk.yellow to give a better warning message:
console.warn(`${chalk.yellow('global installation for create-react-app is deprecated.')}`);
There was a problem hiding this comment.
I don't agree @Taym95, as this is Node, not the browser.
But I do agree that we should use colouring! :)
A few other thoughts:
- Perhaps this should be the first thing emitted from the script? Above
program? - Could the copy be more descriptive? Something like:
Global installations of
create-react-appare now deprecated.To remove the globally installed package, run
npm uninstall -g create-react-app.Updated installation instructions can be found here: https://facebook.github.io/create-react-app/docs/getting-started#quick-start
Edit: Actually @Taym95, I've noted that we do use console.warn in other places in our Node scripts - so that's a great suggestion ;)
There was a problem hiding this comment.
We may need a short URL for that - @ianschmitz, do you know if we have one?
There was a problem hiding this comment.
Not to my knowledge @mrmckeb.
| @@ -40,6 +40,7 @@ const dns = require('dns'); | |||
| const envinfo = require('envinfo'); | |||
| const execSync = require('child_process').execSync; | |||
| const fs = require('fs-extra'); | |||
| const isInstalledGlobally = require('is-installed-globally'); | |||
There was a problem hiding this comment.
Is there a simple way to achieve this without an extra dependency?
There was a problem hiding this comment.
We need to get a current global path for npm or yarn and check it against the current executing path for npm or yarn.
Not sure if it possible internally without dependency in create-react-app script.
| @@ -124,6 +125,10 @@ const program = new commander.Command(packageJson.name) | |||
| }) | |||
| .parse(process.argv); | |||
|
|
|||
| if (isInstalledGlobally) { | |||
| console.log(`global installation for create-react-app is deprecated.`); | |||
There was a problem hiding this comment.
I don't agree @Taym95, as this is Node, not the browser.
But I do agree that we should use colouring! :)
A few other thoughts:
- Perhaps this should be the first thing emitted from the script? Above
program? - Could the copy be more descriptive? Something like:
Global installations of
create-react-appare now deprecated.To remove the globally installed package, run
npm uninstall -g create-react-app.Updated installation instructions can be found here: https://facebook.github.io/create-react-app/docs/getting-started#quick-start
Edit: Actually @Taym95, I've noted that we do use console.warn in other places in our Node scripts - so that's a great suggestion ;)
| @@ -124,6 +125,10 @@ const program = new commander.Command(packageJson.name) | |||
| }) | |||
| .parse(process.argv); | |||
|
|
|||
| if (isInstalledGlobally) { | |||
| console.log(`global installation for create-react-app is deprecated.`); | |||
There was a problem hiding this comment.
We may need a short URL for that - @ianschmitz, do you know if we have one?
|
Based on maintainers feedback, this needs to check for |
fixes #7422
Add global installation deprecate warning when create-react-app runs.