From d2e2efb170544bde7e7214ff861c5e86981a81fb Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Fri, 27 Dec 2019 19:42:28 -0500 Subject: [PATCH 01/11] Fix #8655, skip rendering meta tags with undefined props --- packages/next/next-server/lib/head.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index f9aad9a57b13b..e4042441af990 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -91,6 +91,7 @@ function unique() { for (let i = 0, len = METATYPES.length; i < len; i++) { const metatype = METATYPES[i] if (!h.props.hasOwnProperty(metatype)) continue + if (Object.values(h.props).includes(undefined)) return if (metatype === 'charSet') { if (metaTypes.has(metatype)) { From 69c19b77baa97068f9fee34840917b71a5ca5602 Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Fri, 27 Dec 2019 19:50:47 -0500 Subject: [PATCH 02/11] Filter all tags, not just meta --- packages/next/next-server/lib/head.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index e4042441af990..c143ec34a3b3d 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -66,6 +66,7 @@ function unique() { const metaCategories: { [metatype: string]: Set } = {} return (h: React.ReactElement) => { + if (Object.values(h.props).includes(undefined)) return let unique = true if (h.key && typeof h.key !== 'number' && h.key.indexOf('$') > 0) { @@ -91,7 +92,6 @@ function unique() { for (let i = 0, len = METATYPES.length; i < len; i++) { const metatype = METATYPES[i] if (!h.props.hasOwnProperty(metatype)) continue - if (Object.values(h.props).includes(undefined)) return if (metatype === 'charSet') { if (metaTypes.has(metatype)) { From 7a35d8891873bf95c4c236b555217904924da06a Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Sat, 28 Dec 2019 21:31:03 -0500 Subject: [PATCH 03/11] Only render defined props --- packages/next/next-server/lib/head.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index c143ec34a3b3d..54b912f4b3492 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -66,7 +66,6 @@ function unique() { const metaCategories: { [metatype: string]: Set } = {} return (h: React.ReactElement) => { - if (Object.values(h.props).includes(undefined)) return let unique = true if (h.key && typeof h.key !== 'number' && h.key.indexOf('$') > 0) { @@ -141,8 +140,11 @@ function reduceComponents( .filter(unique()) .reverse() .map((c: React.ReactElement, i: number) => { - const key = c.key || i - return React.cloneElement(c, { key }) + let props: { [key: string]: any } = { key: c.key || i } + Object.entries(c.props).forEach(([key, val]) => { + if (![val, typeof val].includes('undefined')) props[key] = val + }) + return React.cloneElement(c, props) }) } From c247ee6170fcbff5c8cdb19fbe52b9c4a07049c5 Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Mon, 30 Dec 2019 14:03:55 -0500 Subject: [PATCH 04/11] Remove filtering of undefined strings Co-Authored-By: Tim Neutkens --- packages/next/next-server/lib/head.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index 54b912f4b3492..3a204743ff25e 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -142,7 +142,7 @@ function reduceComponents( .map((c: React.ReactElement, i: number) => { let props: { [key: string]: any } = { key: c.key || i } Object.entries(c.props).forEach(([key, val]) => { - if (![val, typeof val].includes('undefined')) props[key] = val + if (typeof val !== 'undefined') props[key] = val }) return React.cloneElement(c, props) }) From ec22d86087cf02824fd188148ffb0ff0c11b4205 Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Mon, 6 Jan 2020 14:38:20 -0500 Subject: [PATCH 05/11] Replace Object.entries --- packages/next/next-server/lib/head.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index 3a204743ff25e..0afa43f4ad9ba 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -141,8 +141,8 @@ function reduceComponents( .reverse() .map((c: React.ReactElement, i: number) => { let props: { [key: string]: any } = { key: c.key || i } - Object.entries(c.props).forEach(([key, val]) => { - if (typeof val !== 'undefined') props[key] = val + Object.keys(c.props).forEach(key => { + if (typeof c.props[key] !== 'undefined') props[key] = c.props[key] }) return React.cloneElement(c, props) }) From 08ab0070d1f44576f547773ec409a848c6ccb353 Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Mon, 6 Jan 2020 14:46:58 -0500 Subject: [PATCH 06/11] Remove filtering code --- packages/next/next-server/lib/head.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index 0afa43f4ad9ba..9430a2f28267b 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -140,10 +140,7 @@ function reduceComponents( .filter(unique()) .reverse() .map((c: React.ReactElement, i: number) => { - let props: { [key: string]: any } = { key: c.key || i } - Object.keys(c.props).forEach(key => { - if (typeof c.props[key] !== 'undefined') props[key] = c.props[key] - }) + let props: { [key: string]: any } = { key: c.key || i, ...c.props } return React.cloneElement(c, props) }) } From a77ffeb0bd6face0f52e23619222bb82050e51dc Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Tue, 7 Jan 2020 17:35:58 -0500 Subject: [PATCH 07/11] Simplify code --- packages/next/next-server/lib/head.tsx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index 9430a2f28267b..f6447fc3c3c3b 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -139,10 +139,9 @@ function reduceComponents( .concat(defaultHead(props.inAmpMode)) .filter(unique()) .reverse() - .map((c: React.ReactElement, i: number) => { - let props: { [key: string]: any } = { key: c.key || i, ...c.props } - return React.cloneElement(c, props) - }) + .map((c: React.ReactElement, i: number) => + React.cloneElement(c, { key: c.key || i, ...c.props }) + ) } const Effect = withSideEffect() From 69aef18a5ac6c965bc90e072da8e388a85e633e1 Mon Sep 17 00:00:00 2001 From: Lachlan Campbell Date: Thu, 30 Jan 2020 21:16:59 -0500 Subject: [PATCH 08/11] Add test --- test/integration/client-navigation/pages/head.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/integration/client-navigation/pages/head.js b/test/integration/client-navigation/pages/head.js index 6b868ec0de995..141611ec6e421 100644 --- a/test/integration/client-navigation/pages/head.js +++ b/test/integration/client-navigation/pages/head.js @@ -16,6 +16,9 @@ export default () => ( + {/* this will not render */} + + {/* allow duplicates for specific tags */} From ddbd3c56057792a42e741de344fa73a1f80d5816 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Sun, 2 Feb 2020 12:02:07 -0600 Subject: [PATCH 09/11] Add tests for undefined head prop value and tweak check --- packages/next/next-server/lib/head.tsx | 18 +++++++++++++++--- .../client-navigation/pages/head.js | 2 +- .../client-navigation/test/index.test.js | 9 +++++++++ .../client-navigation/test/rendering.js | 8 ++++++++ 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index f6447fc3c3c3b..124b88bd68da6 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -139,9 +139,21 @@ function reduceComponents( .concat(defaultHead(props.inAmpMode)) .filter(unique()) .reverse() - .map((c: React.ReactElement, i: number) => - React.cloneElement(c, { key: c.key || i, ...c.props }) - ) + .map((c: React.ReactElement, i: number) => { + const cleanProps: { [key: string]: string } = {} + + Object.keys(c.props).forEach(key => { + let val = c.props[key] + + // TODO: do we want to normalize null/true/false here also? + if (val === undefined) { + val = '' + } + cleanProps[key] = val + }) + + return React.cloneElement(c, { key: c.key || i, ...cleanProps }) + }) } const Effect = withSideEffect() diff --git a/test/integration/client-navigation/pages/head.js b/test/integration/client-navigation/pages/head.js index 141611ec6e421..62c41a9f60a13 100644 --- a/test/integration/client-navigation/pages/head.js +++ b/test/integration/client-navigation/pages/head.js @@ -17,7 +17,7 @@ export default () => ( {/* this will not render */} - + {/* allow duplicates for specific tags */} diff --git a/test/integration/client-navigation/test/index.test.js b/test/integration/client-navigation/test/index.test.js index c236e8bd249ac..2ae59444ed847 100644 --- a/test/integration/client-navigation/test/index.test.js +++ b/test/integration/client-navigation/test/index.test.js @@ -1097,6 +1097,15 @@ describe('Client Navigation', () => { await browser.close() }) + it('should handle undefined prop in head client-side', async () => { + const browser = await webdriver(context.appPort, '/head') + const value = await browser + .elementByCss('meta[name="empty-content"]') + .getAttribute('content') + + expect(value).toBe('') + }) + renderingSuite( (p, q) => renderViaHTTP(context.appPort, p, q), (p, q) => fetchViaHTTP(context.appPort, p, q) diff --git a/test/integration/client-navigation/test/rendering.js b/test/integration/client-navigation/test/rendering.js index b7679fbb45796..9d0986e822111 100644 --- a/test/integration/client-navigation/test/rendering.js +++ b/test/integration/client-navigation/test/rendering.js @@ -17,6 +17,14 @@ export default function(render, fetch) { expect(html.includes('My component!')).toBeTruthy() }) + it('should handle undefined prop in head server-side', async () => { + const html = await render('/head') + const $ = cheerio.load(html) + const value = $('meta[name="empty-content"]').attr('content') + + expect(value).toBe('') + }) + test('renders with fragment syntax', async () => { const html = await render('/fragment-syntax') expect(html.includes('My component!')).toBeTruthy() From dff4aa5773b48c8c3372616acfea5f260e8284d2 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Mon, 3 Feb 2020 13:34:58 -0600 Subject: [PATCH 10/11] Update to strip undefined prop values to match react --- packages/next/client/head-manager.js | 3 +++ packages/next/next-server/lib/head.tsx | 15 ++------------- .../client-navigation/test/index.test.js | 8 ++++---- .../client-navigation/test/rendering.js | 4 ++-- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/packages/next/client/head-manager.js b/packages/next/client/head-manager.js index 865f50bc52a88..5eec36503c0ce 100644 --- a/packages/next/client/head-manager.js +++ b/packages/next/client/head-manager.js @@ -95,6 +95,9 @@ function reactElementToDOM({ type, props }) { if (!props.hasOwnProperty(p)) continue if (p === 'children' || p === 'dangerouslySetInnerHTML') continue + // we don't render undefined props to the DOM + if (props[p] === undefined) continue + const attr = DOMAttributeNames[p] || p.toLowerCase() el.setAttribute(attr, props[p]) } diff --git a/packages/next/next-server/lib/head.tsx b/packages/next/next-server/lib/head.tsx index 124b88bd68da6..f9aad9a57b13b 100644 --- a/packages/next/next-server/lib/head.tsx +++ b/packages/next/next-server/lib/head.tsx @@ -140,19 +140,8 @@ function reduceComponents( .filter(unique()) .reverse() .map((c: React.ReactElement, i: number) => { - const cleanProps: { [key: string]: string } = {} - - Object.keys(c.props).forEach(key => { - let val = c.props[key] - - // TODO: do we want to normalize null/true/false here also? - if (val === undefined) { - val = '' - } - cleanProps[key] = val - }) - - return React.cloneElement(c, { key: c.key || i, ...cleanProps }) + const key = c.key || i + return React.cloneElement(c, { key }) }) } diff --git a/test/integration/client-navigation/test/index.test.js b/test/integration/client-navigation/test/index.test.js index 2ae59444ed847..81aab25d0b341 100644 --- a/test/integration/client-navigation/test/index.test.js +++ b/test/integration/client-navigation/test/index.test.js @@ -1099,11 +1099,11 @@ describe('Client Navigation', () => { it('should handle undefined prop in head client-side', async () => { const browser = await webdriver(context.appPort, '/head') - const value = await browser - .elementByCss('meta[name="empty-content"]') - .getAttribute('content') + const value = await browser.eval( + `document.querySelector('meta[name="empty-content"]').hasAttribute('content')` + ) - expect(value).toBe('') + expect(value).toBe(false) }) renderingSuite( diff --git a/test/integration/client-navigation/test/rendering.js b/test/integration/client-navigation/test/rendering.js index 9d0986e822111..4bc7ef238d89d 100644 --- a/test/integration/client-navigation/test/rendering.js +++ b/test/integration/client-navigation/test/rendering.js @@ -20,9 +20,9 @@ export default function(render, fetch) { it('should handle undefined prop in head server-side', async () => { const html = await render('/head') const $ = cheerio.load(html) - const value = $('meta[name="empty-content"]').attr('content') + const value = 'content' in $('meta[name="empty-content"]').attr() - expect(value).toBe('') + expect(value).toBe(false) }) test('renders with fragment syntax', async () => { From 57b74080588279da941779e98e61d3185c34e1c0 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Mon, 3 Feb 2020 14:41:05 -0500 Subject: [PATCH 11/11] Update head.js --- test/integration/client-navigation/pages/head.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/client-navigation/pages/head.js b/test/integration/client-navigation/pages/head.js index 62c41a9f60a13..ae698dc67e70e 100644 --- a/test/integration/client-navigation/pages/head.js +++ b/test/integration/client-navigation/pages/head.js @@ -16,7 +16,7 @@ export default () => ( - {/* this will not render */} + {/* this will not render the content prop */} {/* allow duplicates for specific tags */}