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

[Documentation] README.md suggests "npm install --save-dev react-admin" instead of "npm install react-admin" #2420

Closed
ajhool opened this issue Oct 11, 2018 · 7 comments

Comments

@ajhool
Copy link
Contributor

ajhool commented Oct 11, 2018

This is a minor documentation issue so I am ignoring the standard bug report for brevity's sake. Happy to edit to the full format if need be!

The README.md installation section:

Installation
React-admin is available from npm. You can install it (and its required dependencies) using:

npm install --save-dev react-admin

react-admin is a runtime package, so why is --save-dev suggested?

Suggested update: change README.md to use npm install react-admin or yarn add react-admin instead of npm install --save-dev react-admin

@phacks
Copy link
Contributor

phacks commented Nov 21, 2018

I was asking myself the exact same question. Did a little bit of digging and it looks like the --save-dev dates back to the early days of admin-on-rest, 2+ years ago.

Maybe @fzaninotto knows the reason?

@fzaninotto
Copy link
Member

It dates from my confusion about how React apps are built: since we use webpack at build time, and webpack creates a bundle with all the required dependencies, I thought there were no need for react or react-admin to actually run the app. But dependencies for SPAs aren't about running the app, but about building the app.

So yes, you're right, we can remove --save-dev. Would you like to open a PR?

@phacks
Copy link
Contributor

phacks commented Nov 21, 2018

@fzaninotto thanks for the explanation! I wouldn’t mind opening a PR, but maybe @ajhool might want to give it a go since they spotted this in the first place. What do you think @ajhool?

@ajhool
Copy link
Contributor Author

ajhool commented Nov 21, 2018 via email

@phacks
Copy link
Contributor

phacks commented Nov 26, 2018

@ajhool did you have time to have a look at updating the docs?

@ajhool
Copy link
Contributor Author

ajhool commented Nov 26, 2018

Thanks for reminder, see below :)
#2574

@fzaninotto
Copy link
Member

fixed by #2574

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

No branches or pull requests

3 participants