Fix props rendered as attributes (#61)

* API refactor for avoiding rendering props as DOM attributes * Readme updates * Upgrade deps & docs * Update docs * Update docs
icd2k3 · Mar 10, 2019 · 8719550 · 8719550
1 parent d0142ce
commit 8719550
Show file tree

Hide file tree

Showing 6 changed files with 959 additions and 959 deletions.
diff --git a/MIGRATIONS.md b/MIGRATIONS.md
@@ -1,52 +1,28 @@
-## Migrating from 1.x.x -> 2.x.x
+## Migrating from 2.x.x -> 3.x.x
 
-#### 1.) `withBreadcrumbs` is now the default export
+#### First things, first...
 
-**1.x.x**
-```js
-import { withBreadcrumbs } from 'react-router-breadcrumbs-hoc';
-```
+`withBreadcrumbs` now returns an array of `Object`s instead of `Component`s:
 
-**2.x.x**
-```js
-import withBreadcrumbs from 'react-router-breadcrumbs-hoc';
+```diff
+-  breadcrumbs.map(breadcrumb)
++  breadcrumbs.map({ breadcrumb })
 ```
 
-#### 2.) The breadcrumbs array returned by the HOC is now _just_ the components. It _used_ to be an array of objects, but I decided this approach was easier to understand and made the implementation code a bit cleaner.
-
-**1.x.x**
-```js
-{breadcrumbs.map(({ breadcrumb, path, match }) => (
-  <span key={path}>
-    <NavLink to={match.url}>
-      {breadcrumb}
-    </NavLink>
-  </span>
-))}
-```
+Within this object, other props like `match`, `location`, and pass-through props are also returned:
 
-**2.x.x**
-```js
-{breadcrumbs.map(breadcrumb => (
-  <span key={breadcrumb.props.key}>
-    <NavLink to={breadcrumb.props.match.url}>
-      {breadcrumb}
-    </NavLink>
-  </span>
-))}
+```diff
+-  breadcrumbs.map((breadcrumb) => {})
++  breadcrumbs.map(({ breadcrumb, match, location, someCustomProp }) => {})
 ```
 
-#### 3.) The package will now attempt to return sensible defaults for breadcrumbs unless otherwise provided making the the package now "opt-out" instead of "opt-in" for all paths. See the readme for how to disable default breadcrumb behavior.
+#### Why was this change made?
 
-**1.x.x**
-```js
-withBreadcrumbs([
-  { path: '/', breadcrumb: 'Home' },
-  { path: '/users', breadcrumb: 'Users' },
-])(Component);
-```
+Under the hood, `withBreadcrumbs` uses React's `createElement` method to render breadcrumbs. In version 2, all props (like `match`, `location`, etc) were assigned to the rendered component (for example: `createElement(breadcrumb, componentProps);`).
 
-**2.x.x** (the above breadcrumbs will be automagically generated so there's no need to include them in config)
-```js
-withBreadcrumbs()(Component);
-```
+This had the unintended side-effect of rendering any of these props as an _attribute_ on the DOM element. So, ultimately this resulted in some breadcrumbs rendering like `<span someProp="[Object object]"/>'` as well as some React console warnings [in certain cases](https://github.com/icd2k3/react-router-breadcrumbs-hoc/issues/59).
+
+This issue has been solved by adding the following logic:
+- If the breadcrumb is a simple string, don't render it with props applied
+- If the breadcrumb is a function/class (dynamic), _then_ pass all the props to it
+- Return objects instead of components so that we can still utilize all the props during the `map`
diff --git a/README.md b/README.md
@@ -35,33 +35,48 @@ or
 withBreadcrumbs()(MyComponent);
 ```
 
-## Example
+## Examples
 
+Start seeing generated breadcrumbs right away with this simple example
 ```js
-import React from 'react';
-import { NavLink } from 'react-router-dom';
 import withBreadcrumbs from 'react-router-breadcrumbs-hoc';
 
-// breadcrumbs can be any type of component or string
-const UserBreadcrumb = ({ match }) =>
-  <span>{match.params.userId}</span>; // use match param userId to fetch/display user name
+const Breadcrumbs = ({ breadcrumbs }) => (
+  <React.Fragment>
+    {breadcrumbs.map(({ breadcrumb }) => breadcrumb)}
+  </React.Fragment>
+)
+
+export default withBreadcrumbs()(Breadcrumbs);
+```
+
+The example above will work for some routes, but you may want other routes to be dynamic (such as a user name breadcrumb). Let's modify it to handle custom-set breadcrumbs.
+
+```js
+import withBreadcrumbs from 'react-router-breadcrumbs-hoc';
+
+const userNamesById = { '1': 'John' }
+
+const DynamicUserBreadcrumb = ({ match }) => (
+  <span>{userNamesById[match.params.userId]}</span>
+);
 
-// define some custom breadcrumbs for certain routes (optional)
 const routes = [
-  { path: '/users/:userId', breadcrumb: UserBreadcrumb },
+  { path: '/users/:userId', breadcrumb: DynamicUserBreadcrumb },
   { path: '/example', breadcrumb: 'Custom Example' },
 ];
 
 // map & render your breadcrumb components however you want.
-// each `breadcrumb` has the props `key`, `location`, and `match` included!
 const Breadcrumbs = ({ breadcrumbs }) => (
   <div>
-    {breadcrumbs.map((breadcrumb, index) => (
-      <span key={breadcrumb.key}>
-        <NavLink to={breadcrumb.props.match.url}>
-          {breadcrumb}
-        </NavLink>
-        {(index < breadcrumbs.length - 1) && <i> / </i>}
+    {breadcrumbs.map(({
+      match,
+      breadcrumb
+      // other props are available during render, such as `location` 
+      // and any props found in your route objects will be passed through too
+    }) => (
+      <span key={match.url}>
+        <NavLink to={match.url}>{breadcrumb}</NavLink>
       </span>
     ))}
   </div>
@@ -75,15 +90,34 @@ For the above example...
 Pathname | Result
 --- | ---
 /users | Home / Users
-/users/id | Home / Users / John
+/users/1 | Home / Users / John
 /example | Home / Custom Example
 
 ## Already using a [route config](https://reacttraining.com/react-router/web/example/route-config) array with react-router?
 
 Just add a `breadcrumb` prop to your routes that require custom breadcrumbs.
 
+`{ path, component }` -> `{ path, component, breadcrumb }`
+
 `withBreadcrumbs(routeConfig)(MyComponent)`
 
+## Disabling default generated breadcrumbs
+
+This package will attempt to create breadcrumbs for you based on the route section via [humanize-string](https://github.com/sindresorhus/humanize-string). For example `/users` will auotmatically create the breadcrumb `"Users"`. There are two ways to disable default breadcrumbs for a path:
+
+**Option 1:** Disable _all_ default breadcrumb generation by passing `disableDefaults: true` in the `options` object
+
+`withBreadcrumbs(routes, { disableDefaults: true })`
+
+**Option 2:** Disable _individual_ default breadcrumbs by passing `breadcrumb: null` in route config:
+
+`{ path: '/a/b', breadcrumb: null }`
+
+**Option 3:** Disable _individual_ default breadcrumbs by passing an `excludePaths` array in the `options` object
+
+`withBreadcrumbs(routes, { excludePaths: ['/', '/no-breadcrumb/for-this-route'] })`
+
+
 ## Dynamic breadcrumbs
 
 If you pass a component as the `breadcrumb` prop it will be injected with react-router's [match](https://reacttraining.com/react-router/web/api/match) and [location](https://reacttraining.com/react-router/web/api/location) objects as props. These objects contain ids, hashes, queries, etc from the route that will allow you to map back to whatever you want to display in the breadcrumb.
@@ -124,22 +158,6 @@ const EditorBreadcrumb = ({ location: { state: { isNew } } }) => (
 <Link to={{ pathname: '/editor', state: { isNew: true } }}>Add</Link>
 ```
 
-## Disabling default breadcrumbs for paths
-
-This package will attempt to create breadcrumbs for you based on the route section via [humanize-string](https://github.com/sindresorhus/humanize-string). For example `/users` will auotmatically create the breadcrumb `"Users"`. There are two ways to disable default breadcrumbs for a path:
-
-**Option 1:** Disable _all_ default breadcrumb generation by passing `disableDefaults: true` in the `options` object
-
-`withBreadcrumbs(routes, { disableDefaults: true })`
-
-**Option 2:** Disable _individual_ default breadcrumbs by passing `breadcrumb: null` in route config:
-
-`{ path: '/a/b', breadcrumb: null }`
-
-**Option 3:** Disable _individual_ default breadcrumbs by passing an `excludePaths` array in the `options` object
-
-`withBreadcrumbs(routes, { excludePaths: ['/', '/no-breadcrumb/for-this-route'] })`
-
 ## Order matters!
 
 Consider the following route configs:

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "react-router-breadcrumbs-hoc",
-  "version": "2.3.2",
+  "version": "3.0.0",
   "description": "Just a tiny, flexible, higher order component for rendering breadcrumbs with react-router 4.x",
   "repository": "icd2k3/react-router-breadcrumbs-hoc",
   "keywords": [
@@ -20,38 +20,38 @@
     "react-router-dom": "^4.0.0"
   },
   "dependencies": {
-    "humanize-string": "^1.0.2"
+    "humanize-string": "^2.0.0"
   },
   "devDependencies": {
     "@babel/cli": "^7.1.0",
-    "@babel/core": "^7.1.0",
+    "@babel/core": "^7.3.4",
     "@babel/plugin-transform-modules-commonjs": "^7.1.0",
-    "@babel/preset-env": "^7.1.0",
+    "@babel/preset-env": "^7.3.4",
     "@babel/preset-react": "^7.0.0",
-    "@types/react": "16.8.1",
+    "@types/react": "16.8.7",
     "@types/react-router-dom": "4.3.1",
     "babel-core": "^7.0.0-0",
     "babel-eslint": "^10.0.1",
-    "babel-jest": "^24.0.0",
-    "coveralls": "^3.0.2",
-    "enzyme": "^3.2.0",
-    "enzyme-adapter-react-16": "^1.1.0",
-    "eslint": "^5.1.0",
+    "babel-jest": "^24.3.1",
+    "coveralls": "^3.0.3",
+    "enzyme": "^3.9.0",
+    "enzyme-adapter-react-16": "^1.10.0",
+    "eslint": "^5.15.1",
     "eslint-config-airbnb": "^17.0.0",
     "eslint-plugin-import": "^2.13.0",
-    "eslint-plugin-jsx-a11y": "^6.1.0",
+    "eslint-plugin-jsx-a11y": "^6.2.1",
     "eslint-plugin-react": "^7.10.0",
-    "jest": "^24.0.0",
-    "prop-types": "^15.6.2",
-    "react": "16.7.0",
-    "react-dom": "16.7.0",
+    "jest": "^24.3.1",
+    "prop-types": "^15.7.2",
+    "react": "16.8.4",
+    "react-dom": "16.8.4",
     "react-router": "^4.3.1",
     "react-router-dom": "^4.3.1",
-    "rollup": "^1.1.2",
+    "rollup": "^1.6.0",
     "rollup-plugin-babel": "^4.0.3",
-    "rollup-plugin-commonjs": "^9.1.3",
-    "rollup-plugin-node-resolve": "^4.0.0",
-    "typescript": "^3.0.3"
+    "rollup-plugin-commonjs": "^9.2.1",
+    "rollup-plugin-node-resolve": "^4.0.1",
+    "typescript": "^3.3.3333"
   },
   "scripts": {
     "prepublishOnly": "npm run build",

diff --git a/src/index.js b/src/index.js
@@ -30,30 +30,21 @@ const NO_BREADCRUMB = 'NO_BREADCRUMB';
  * with `match`, `location`, and `key` props.
  */
 const render = ({
-  /**
-   * extracting `component` here to avoid an invalid attribute warning
-   * see: https://github.com/icd2k3/react-router-breadcrumbs-hoc/issues/59
-   * This is actually a symptom of a larger issue with this current
-   * functionality of passing route data (needed for breadcrumb rendering)
-   * as props on the component itself. This has the unintended side-effect
-   * of rendering those props as element attributes in the DOM.
-   * TODO: Refactor this render logic (and probably the API) to not render
-   * those props as attributes on the breadcrumb element.
-   */
-  component,
-
+  component: reactRouterConfigComponent,
   breadcrumb,
   match,
   location,
   ...rest
 }) => {
   const componentProps = { match, location, key: match.url, ...rest };
-  if (typeof breadcrumb === 'function') {
-    return createElement(breadcrumb, componentProps);
-  }
-  return createElement('span', componentProps, breadcrumb);
-};
 
+  return {
+    ...componentProps,
+    breadcrumb: typeof breadcrumb === 'function'
+      ? createElement(breadcrumb, componentProps)
+      : createElement('span', { key: componentProps.key }, breadcrumb),
+  };
+};
 
 /**
  * Small helper method to get a default `humanize-string`
@@ -73,7 +64,7 @@ const getDefaultBreadcrumb = ({ pathSection, currentSection, location }) => {
  * Loops through the route array (if provided) and returns either a
  * user-provided breadcrumb OR a sensible default (if enabled) via `humanize-string`.
 */
-const getBreadcrumb = ({
+const getBreadcrumbMatch = ({
   currentSection,
   disableDefaults,
   excludePaths,
@@ -161,14 +152,14 @@ export const getBreadcrumbs = ({ routes, location, options = {} }) => {
     .replace(/\/$/, '')
     // Split pathname into sections.
     .split('/')
-    // Reduce over the sections and call `getBreadcrumb()` for each section.
+    // Reduce over the sections and call `getBreadcrumbMatch()` for each section.
     .reduce((previousSection, currentSection) => {
       // Combine the last route section with the currentSection.
       // For example, `pathname = /1/2/3` results in match checks for
       // `/1`, `/1/2`, `/1/2/3`.
       const pathSection = !currentSection ? '/' : `${previousSection}/${currentSection}`;
 
-      const breadcrumb = getBreadcrumb({
+      const breadcrumb = getBreadcrumbMatch({
         currentSection,
         location,
         pathSection,