Skip to content
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

Feature/prerender image support #1721

Merged
merged 9 commits into from
Feb 19, 2021

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Feb 3, 2021

Builds on top of #1641

What?

When rendering in the Node.js environment, babel doesn't know how to handle images.

This PR does the following things:

  • handle SVGs using a babel plugin, instead of webpack's loader
  • adds webpack manifest plugin, which generates a manifest.json with all the files that have been processed by webpack
  • adds new babel plugin for prerender, which looks at the manifest.json, and replaces any media files (imgs, fonts,pdfs,etc.) with the path generated by webpack
  • for any files that aren't in the manifest (which means its handled by the url loader), it puts in place a blank transparent gif data url. There's room for improvement here, for sure! But instead of putting the effort here, we should think about how to optimise the loading of images similar to next-images

Why?

So that we can prerender pages where media is imported like

import cuteKittenSrc from './cuteKitten.png'

Other notes

1. I've noticed there are subtle differences in how SVGs are handled. For example, this won't work anymore:

Logo.js

import Logo from './Logo.svg'
export default Logo

but this will

import Logo from './Logo.svg'
const LogoImage = () => <Logo/>

export default LogoImage

@dac09 dac09 requested a review from peterp February 3, 2021 18:15
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/create-redwood-app-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-api-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-api-server-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-auth-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-cli-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-core-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-dev-server-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-eslint-config-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-eslint-plugin-redwood-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-forms-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-internal-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-prerender-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-router-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-structure-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-testing-0.25.1-40e55f8.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1721/redwoodjs-web-0.25.1-40e55f8.tgz

Install this PR by running yarn rw upgrade --pr 1721:0.25.1-40e55f8

@dac09
Copy link
Collaborator Author

dac09 commented Feb 17, 2021

Converting back to draft, I will rebase the relevant code after merge of #1641

@dac09 dac09 marked this pull request as draft February 17, 2021 12:59
@dac09 dac09 force-pushed the feature/prerender-image-support branch from 76ba73a to c1b4cf8 Compare February 18, 2021 16:34
@dac09 dac09 force-pushed the feature/prerender-image-support branch from c1b4cf8 to 425a25a Compare February 18, 2021 16:35
@dac09
Copy link
Collaborator Author

dac09 commented Feb 18, 2021

@peterp ready for review.

@dac09 dac09 marked this pull request as ready for review February 18, 2021 16:48
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, just a few changes to the package.json files.

I'm a little concerned about the having to wrap the SVG imports into functions, since that would introduce a breaking change? As an example:

import MyIcon from './MyIcon.svg'

export const MyComponent = () => {
   <div>
       <MyIcon />
   </div>
}

Would this not work anymore? It's odd to me, since the documentation seems to indicate that it creates a component.

If this is the case then this is a huge breaking change.

packages/core/config/webpack.common.js Show resolved Hide resolved
packages/prerender/package.json Outdated Show resolved Hide resolved
packages/prerender/package.json Outdated Show resolved Hide resolved
packages/prerender/package.json Outdated Show resolved Hide resolved
@thedavidprice
Copy link
Contributor

@dac09 I took this for a spin locally. (Note: I'm having a lot of trouble these days with my local dev setup. PR Packages don't seem to be created correctly, i.e. the inter-dependencies aren't actually to the PR package versions. Verdaccio was failing. And rwt cpw is hit or miss. No idea...)

Got this code to prerender:

// .../BlogLayout.js
...

import FaviconImage from './favicon.png'

const Favicon = () => <FaviconImage />

const BlogLayout = ({ children }) => {
  const { logIn, logOut, isAuthenticated, currentUser } = useAuth()

  return (
    <>
      <header>
        <h1>
          <img src={Favicon} /> <Link to={routes.home()}>Redwood Blog</Link>
        </h1>
...

Ran rw build successfully.

But when I ran rw prerender -d, I am getting this:

Could not resolve "/Users/price/Repos/tdp-tutorial-blog-test/web/src/pages/FatalErrorPage" in file /Users/price/Repos/tdp-tutorial-blog-test/web/src/index.js.
Could not resolve "/Users/price/Repos/tdp-tutorial-blog-test/web/src/layouts/BlogLayout" in file /Users/price/Repos/tdp-tutorial-blog-test/web/src/pages/AboutPage/AboutPage.js.
Warning: Invalid value for prop `src` on <img> tag. Either remove it from the element, or pass a string or number value to keep it in the DOM. For details, see https://fb.me/react-attribute-behavior
...
::: Dry run, not writing changes :::
::: 🚀 Prerender output for /about ::: 
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width,initial-scale=1" />
    <link rel="icon" type="image/png" href="/favicon.png" />
    <title>tdp-tutorial-blog-test</title>
    <script defer="defer" src="/static/js/runtime-app.31ca6fe6.js"></script>
    <script defer="defer" src="/static/js/vendors.9e95d691.chunk.js"></script>
    <script defer="defer" src="/static/js/app.87f08f92.chunk.js"></script>
    <link href="/static/css/app.6d4df765.css" rel="stylesheet" />
  </head>
  <body>
    <div id="redwood-app">
      <header>
        <h1><img /> <a href="/">Redwood Blog</a></h1>
...

@dac09
Copy link
Collaborator Author

dac09 commented Feb 19, 2021

This looks good to me, just a few changes to the package.json files.

I'm a little concerned about the having to wrap the SVG imports into functions, since that would introduce a breaking change? As an example:

import MyIcon from './MyIcon.svg'



export const MyComponent = () => {

   <div>

       <MyIcon />

   </div>

}

Would this not work anymore? It's odd to me, since the documentation seems to indicate that it creates a component.

If this is the case then this is a huge breaking change.

@peterp that would definitely work. It's only when you are "re-exporting" an image that it doesn't work.

…dwood into feature/prerender-image-support

* 'feature/prerender-image-support' of github.com:dac09/redwood:
  Update yarn.lock
  use 'prisma' in place of '@prisma/cli' (redwoodjs#1800)
@dac09
Copy link
Collaborator Author

dac09 commented Feb 19, 2021

Got this code to prerender:

// .../BlogLayout.js
...

import FaviconImage from './favicon.png'

const Favicon = () => <FaviconImage />

const BlogLayout = ({ children }) => {
  const { logIn, logOut, isAuthenticated, currentUser } = useAuth()

  return (
    <>
      <header>
        <h1>
          <img src={Favicon} /> <Link to={routes.home()}>Redwood Blog</Link>
        </h1>
...

Ran rw build successfully.

But when I ran rw prerender -d, I am getting this:

There's a syntax error there, the way imported images work (unless they're svg) is:

import FaviconImage from './favicon.png'

return (
    <>
      <header>
        <h1>
          <img src={FaviconImage} /> <Link to={routes.home()}>Redwood Blog</Link>
        </h1>
 ...

So you can skip that extra Favicon component :)

@dac09 dac09 merged commit d57003a into redwoodjs:main Feb 19, 2021
@dac09 dac09 deleted the feature/prerender-image-support branch February 19, 2021 13:11
@thedavidprice thedavidprice added this to the next release milestone Feb 19, 2021
@thedavidprice
Copy link
Contributor

Ah, got it. OK, gonna need more. Doc. POWER 🦾⚡️

@dac09
Copy link
Collaborator Author

dac09 commented Feb 19, 2021

I think its documented somewhere, definitely seen it before. Defo worth exposing a bit more though

its maintaining the behaviour from before, not introducing something new!

dac09 added a commit to dac09/redwood that referenced this pull request Feb 22, 2021
…rib-workspaces

* 'main' of github.com:redwoodjs/redwood:
  Lint the create-redwood-app template. (redwoodjs#1822)
  Update typescript lint rule for no-unused-vars (redwoodjs#1808)
  Router: Fix TS types (redwoodjs#1823)
  Generate globals for routes (redwoodjs#1744)
  Ethereum auth update to v0.2.1 (redwoodjs#1807)
  Better messages when no routes marked with prerender (redwoodjs#1821)
  Feature/prerender image support (redwoodjs#1721)
  Fix entry js | Make index a little more readable, as it confuses people (redwoodjs#1819)
  use 'prisma' in place of '@prisma/cli' (redwoodjs#1800)
jtoar pushed a commit that referenced this pull request Dec 22, 2022
* chore(deps): remove svg-react-loader

svg-react-loader doesn't appear to be used as of
d57003a (Feature/prerender image support (#1721), 2021-02-19)
and depends on a vulnerable version of loader-utils
(see CVE-2022-37601)

* chore: update yarn.lock
github-actions bot pushed a commit that referenced this pull request Dec 22, 2022
* chore(deps): remove svg-react-loader

svg-react-loader doesn't appear to be used as of
d57003a (Feature/prerender image support (#1721), 2021-02-19)
and depends on a vulnerable version of loader-utils
(see CVE-2022-37601)

* chore: update yarn.lock
jtoar pushed a commit that referenced this pull request Dec 23, 2022
* chore(deps): remove svg-react-loader

svg-react-loader doesn't appear to be used as of
d57003a (Feature/prerender image support (#1721), 2021-02-19)
and depends on a vulnerable version of loader-utils
(see CVE-2022-37601)

* chore: update yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants