Incorporated development + production builds into /dist (#64) #66
Conversation
updated Readme to include CDN/External information.
virgofx
changed the title
| ## High-level API: CSSTransitionGroup | ||
|
|
||
| `CSSTransitionGroup` is a high-level API based on [`TransitionGroup`](#low-level-api-transitiongroup) and is an easy way to perform CSS transitions and animations when a React component enters or leaves the DOM. It's inspired by the excellent [ng-animate](http://www.nganimate.org/) library. | ||
|
|
||
| **Importing** | ||
|
|
||
| ```javascript | ||
| import CSSTransitionGroup from 'react-transition-group/CSSTransitionGroup' // ES6 | ||
| import { CSSTransitionGroup } from 'react-transition-group' // ES6 |
There was a problem hiding this comment.
I'd rather show the cherry picked version here :)
There was a problem hiding this comment.
I wasn't able to get the cherry picked version to exclude properly when testing. (Which is why I included the corresponding webpack config in the readme as well)
All documentation everywhere prefers the standard ES6 brace style for non-default components. Do you have an example webpack external config where using cherry picked version will work?
There was a problem hiding this comment.
Ya you can't cherry pick from the dist builds since they are flat files (a downside of UMD builds). We really should just both options since they are all valid choices for importing. I'm fine leaving this change in. I can make the docs more detailed later
There was a problem hiding this comment.
Gotcha. Dropped the webpack build specific option above and added minor clarity. If you want to clarify the import or even better document both (Cherry picked for direct bundled or ES6 style import for those using /dist builds), fine with me.
README.md
Outdated
| to the following CDN: | ||
|
|
||
| [https://unpkg.com/react-transition-group/dist/react-transition-group.min.js](https://unpkg.com/react-transition-group/dist/react-transition-group.min.js) | ||
|
|
There was a problem hiding this comment.
I appreciate the extra documentation, however in this case I don't to give the external consumption version of the library such a prominent place in the docs, as it's not really recommended. In other words, most folks shouldn't use the CDN version, but instead include this in a vendor bundle (maybe put on a cdn) via webpack.
I'd just make a quick note that for folks that do need a cdn version they can use unpkg.com
There was a problem hiding this comment.
Various libraries exclude this package but depend on it. I'd argue that library components absolutely should be used from a CDN as the alternative reinforces bundling huge apps, SPA, large bundles with users not sure how to split/reuse common framework components. E.g. React, Bootstrap, etc. This is an extension of React essentially.
Nonetheless, would you prefer I move to the bottom or remove?
There was a problem hiding this comment.
externalizing dependencies is balance between size, usage and geographic location. Libraries like React may make sense to consume from a CDN since it's fairly large, and fairly likely to already be in a user's cache since lots of sites use it. Smaller libraries like this are way less likely to already be cached and are so small I'd argue the latency of a new network request outweighs the benefit of making it external. Not to mention request limit bottlenecks in browsers.
That said folks should measure and pick for their use cases, but I think generally making all libraries external isn't great default advice and I'd rather just stay out of that game :) to that end we can keep the CDN and dist build info but let's remove the webpack config details since that's more build specific
|
@jquense Thanks for the quick feedback. I'll try to update soon as I get your latest answers to my last Qs! |
and adding clarity for CDN/external.
|
Thanks! |
* Incorporated development + production builds into /dist (#64) and updated Readme to include CDN/External information. * Dropping references to Webpack specific build configurations in Readme and adding clarity for CDN/external.
* Initial * clean up prop names * more exploration * fun children * fix tests * some comment docs * Incorporated development + production builds into /dist (#64) (#66) * Incorporated development + production builds into /dist (#64) and updated Readme to include CDN/External information. * Dropping references to Webpack specific build configurations in Readme and adding clarity for CDN/external. * Enhancement for best practice prop types. (#70) * Fixes #69: Adding capability to reduce production build output size and allow users that import this package to include prop types in development and optionally remove them using Webpacks UglifyJS + Define plugin. * Updated .babelrc that properly wraps prop-types. * Restoring the original constant declaration for prop types as uglify does find dead code and remove it. * Updating changelog.
Incorporated development + production builds into /dist (#64) and updated Readme to include CDN/External information.