Add error message for AMP bind syntax (vercel#6865)

* Add error message for AMP bind syntax * Fix custom AMP scripts getting dropped * Add data.js to package.json to include it in releases
filoblu · Apr 2, 2019 · aac50e4 · aac50e4
1 parent 4201fb9
commit aac50e4
Show file tree

Hide file tree

Showing 7 changed files with 99 additions and 42 deletions.
diff --git a/errors/amp-bind-jsx-alt.md b/errors/amp-bind-jsx-alt.md
@@ -0,0 +1,14 @@
+# AMP Bind JSX Error
+
+#### Why This Error Occurred
+
+Somewhere you used `[prop]='something'` syntax which is not allowed in JSX.
+
+#### Possible Ways to Fix It
+
+Instead you can use `data-amp-bind-prop='something'` which is a valid AMP alternative
+
+### Useful Links
+
+- [AMP HTML Specification](https://www.ampproject.org/docs/fundamentals/spec)
+- [Possible future syntax](https://github.com/ampproject/amphtml/issues/21600)
diff --git a/packages/next-server/server/render.tsx b/packages/next-server/server/render.tsx
@@ -287,21 +287,27 @@ export async function renderToHTML(
     ? renderToStaticMarkup
     : renderToString
 
+  const renderPageError = (): {html: string, head: any} | void => {
+    if (ctx.err && ErrorDebug) {
+      return render(renderElementToString, <ErrorDebug error={ctx.err} />)
+    }
+
+    if (dev && (props.router || props.Component)) {
+      throw new Error(
+        `'router' and 'Component' can not be returned in getInitialProps from _app.js https://err.sh/zeit/next.js/cant-override-next-props.md`,
+      )
+    }
+  }
+
   let renderPage: (options: ComponentsEnhancer) => { html: string, head: any } | Promise<{ html: string; head: any }>
 
   if (ampBindInitData) {
     renderPage = async (
       options: ComponentsEnhancer = {},
     ): Promise<{ html: string; head: any }> => {
-      if (ctx.err && ErrorDebug) {
-        return render(renderElementToString, <ErrorDebug error={ctx.err} />)
-      }
+      const renderError = renderPageError()
+      if (renderError) return renderError
 
-      if (dev && (props.router || props.Component)) {
-        throw new Error(
-          `'router' and 'Component' can not be returned in getInitialProps from _app.js https://err.sh/zeit/next.js/cant-override-next-props.md`,
-        )
-      }
       const {
         App: EnhancedApp,
         Component: EnhancedComponent,
@@ -348,40 +354,33 @@ export async function renderToHTML(
     }
   } else {
     renderPage = (
-    options: ComponentsEnhancer = {},
-  ): { html: string; head: any } => {
-    if (ctx.err && ErrorDebug) {
-      return render(renderElementToString, <ErrorDebug error={ctx.err} />)
-    }
+      options: ComponentsEnhancer = {},
+    ): { html: string; head: any } => {
+      const renderError = renderPageError()
+      if (renderError) return renderError
 
-    if (dev && (props.router || props.Component)) {
-      throw new Error(
-        `'router' and 'Component' can not be returned in getInitialProps from _app.js https://err.sh/zeit/next.js/cant-override-next-props.md`,
+      const {
+        App: EnhancedApp,
+        Component: EnhancedComponent,
+      } = enhanceComponents(options, App, Component)
+
+      return render(
+        renderElementToString,
+        <RouterContext.Provider value={router}>
+          <IsAmpContext.Provider value={amphtml}>
+            <LoadableContext.Provider
+              value={(moduleName) => reactLoadableModules.push(moduleName)}
+            >
+              <EnhancedApp
+                Component={EnhancedComponent}
+                router={router}
+                {...props}
+              />
+            </LoadableContext.Provider>
+          </IsAmpContext.Provider>
+        </RouterContext.Provider>,
       )
     }
-
-    const {
-      App: EnhancedApp,
-      Component: EnhancedComponent,
-    } = enhanceComponents(options, App, Component)
-
-    return render(
-      renderElementToString,
-      <RouterContext.Provider value={router}>
-        <IsAmpContext.Provider value={amphtml}>
-          <LoadableContext.Provider
-            value={(moduleName) => reactLoadableModules.push(moduleName)}
-          >
-            <EnhancedApp
-              Component={EnhancedComponent}
-              router={router}
-              {...props}
-            />
-          </LoadableContext.Provider>
-        </IsAmpContext.Provider>
-      </RouterContext.Provider>,
-    )
-  }
   }
 
   const docProps = await loadGetInitialProps(Document, { ...ctx, renderPage })
@@ -418,7 +417,7 @@ export async function renderToHTML(
     devFiles,
   })
 
-  if (amphtml && html) {
+  if (!dev && amphtml && html) {
     html = await optimizeAmp(html, { amphtml, noDirtyAmp, query })
   }
   return html

diff --git a/packages/next/build/output/store.ts b/packages/next/build/output/store.ts
@@ -2,6 +2,7 @@ import chalk from 'chalk'
 import createStore from 'next/dist/compiled/unistore'
 import readline from 'readline'
 import { onExit } from './exit'
+import stripAnsi from 'strip-ansi'
 
 export type OutputState =
   | { bootstrap: true; appUrl: string | null }
@@ -39,10 +40,22 @@ store.subscribe(state => {
     console.log('Compiling ...')
     return
   }
-
+  
   if (state.errors) {
     console.log(chalk.red('Failed to compile.'))
     console.log()
+    const cleanError = stripAnsi(state.errors[0])
+    if (cleanError.indexOf('SyntaxError') > -1) {
+      const matches = cleanError.match(/\[.*\]=/)
+      if (matches) {
+        for (const match of matches) {
+          const prop = (match.split(']').shift() || '').substr(1)
+          console.log(`AMP bind syntax [${prop}]='' is not supported in JSX, use 'data-amp-bind-${prop}' instead. https://err.sh/zeit/next.js/amp-bind-jsx-alt`)
+        }
+        console.log()
+        return
+      }
+    }
     console.log(state.errors[0])
     return
   }

diff --git a/packages/next/package.json b/packages/next/package.json
@@ -14,6 +14,7 @@
     "client.js",
     "config.js",
     "constants.js",
+    "data.js",
     "document.js",
     "dynamic.js",
     "error.js",

diff --git a/packages/next/pages/_document.js b/packages/next/pages/_document.js
@@ -187,7 +187,7 @@ export class Head extends Component {
         badProp = 'name="viewport"'
       } else if (type === 'link' && props.rel === 'canonical') {
         badProp = 'rel="canonical"'
-      } else if (type === 'script') {
+      } else if (type === 'script' && !(props.src && props.src.indexOf('ampproject') > -1)) {
         badProp = '<script'
         Object.keys(props).forEach(prop => {
           badProp += ` ${prop}="${props[prop]}"`

diff --git a/test/integration/amphtml/pages/amp-script.amp.js b/test/integration/amphtml/pages/amp-script.amp.js
@@ -0,0 +1,25 @@
+import Head from 'next/head'
+
+const date = new Date().toJSON()
+
+export default () => (
+  <>
+    <Head>
+      <script
+        async
+        key='amp-timeago'
+        custom-element='amp-timeago'
+        src='https://cdn.ampproject.org/v0/amp-timeago-0.1.js'
+      />
+    </Head>
+
+    <amp-timeago
+      width='160'
+      height='20'
+      datetime={date}
+      layout='responsive'
+    >
+      {date}
+    </amp-timeago>
+  </>
+)
diff --git a/test/integration/amphtml/test/index.test.js b/test/integration/amphtml/test/index.test.js
@@ -99,6 +99,11 @@ describe('AMP Usage', () => {
       expect(html).not.toMatch(/console\.log("I'm not either :p")'/)
     })
 
+    it('should not drop custom amp scripts', async () => {
+      const html = await renderViaHTTP(appPort, '/amp-script?amp=1')
+      await validateAMP(html)
+    })
+
     it('should optimize dirty when ?amp=1 is not specified', async () => {
       const html = await renderViaHTTP(appPort, '/only-amp')
       await validateAMP(html, true)