-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Clarify installation instructions #1181
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
Clarify installation instructions #1181
Conversation
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert this change if ya want. My text editor stripped out the extra space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do :-)
Let's add an explicit paragraph explaining how to install and use the UMD build.
|
@jmeas Can you add that npmcdn link? Other than that, this looks good! 😄 |
@jmeas Any chance you can get to this? I can cherry pick in and add the npmcdn link for you, if not. |
I'll add it to my to-do list, and try to get to it in a few days. Since all of the builds export UMD, I think there's some good rephrasing that can be done to capture that important detail. |
There's also the counter-vanilla example now that uses npmcdn, which you may way to point to. |
Hmmm..."counter-vanilla"? Not sure if I know what ya mean. |
Ah, right, haha. Cool, and that's a great idea. I'll add a link to that! |
96f7f29
to
b0ac82d
Compare
Mmk, updated with some of the feedback y'all gave. Let me know what ya think, and if there are any other changes you think I should make. |
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These spaces are significant. They force Markdown to make a newline.
I can fix the spaces. That aside, how are the text changes? |
b38696d
to
2a104ce
Compare
The previous instructions may have come across that the only way to use Redux with npm was with a module bundler, like Browserify or Webpack. This reorganizes the text in an effort to more explicitly explain that it's only the extensions require a CJS module bundler.
2a104ce
to
d221deb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the death by a thousand comments...
I'd change "Use of the extensions requires that you’re using npm" back to something like "This assumes you are using npm", so you avoid having "use" and "using" in the same sentence.
But good catch changing "npm package manager" to just "npm". That's like saying "ATM machine" 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the death by a thousand comments...
No problem at all! I want this to be good. I appreciate the thorough feedback.
I'd change "Use of the extensions requires that you’re using npm" back to something like "This assumes you are using npm", so you avoid having "use" and "using" in the same sentence.
I agree that the current wording is awkward. At the same time, I want to say something a little stronger than "this assumes," because that leaves it open that there are other methods (perhaps the CDNs above, or Bower). The existing text (before I made any changes) made it out that these tools can only be used with npm, or only with CommonJS. I just pulled down react-redux, though, and it includes a UMD build all the same. I then checked cdnjs and saw it there, too.
Perhaps the recommendation to stick with npm isn't as strong as it was originally worded, and how I've worded it to be? It looks like the three install methods (npm, Bower, CDNs) may work for all of 3 of these libraries?
If that's the case, I might want to do another restructuring to make that more clear.
Another thought is that maybe the original wording was referring to popular React libraries outside of the mini-Redux ecosystem described here. Is that the case?
Sorry I'm not more knowledgeable, I haven't used Redux much yet :)
But good catch changing "npm package manager" to just "npm". That's like saying "ATM machine"
Heh heh. Maybe not...
|
||
However, if you don’t yet use [npm](https://www.npmjs.com/) or a modern module bundler, you have a few options. | ||
|
||
##### Bower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don’t want to see us contributing to “Bower is still a good choice for front-end devs” meme by even dedicating it a header. Instead, I would suggest to merge the below two sections, remove cdnjs reference and keep only npmcdn (it’s a better project IMO), and add Bower commend as an afterthought (“If you use an alternative package manager such as Bower, it is possible that your package manager allows installing libraries from URLs, for example: ”). This way it doesn’t look like we encourage it.
This hasn’t been updated in a while so I made some changes inspired by this in 7b85962. |
Thanks @gaearon ✌️ |
When I read the installation instructions, it didn't come across to me that the npm package could be used without a CJS module bundler. I tried to reorganize the instructions here to make it a bit more clear.
The rest of the commit msg: