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

feat(gatsby): add assetPrefix to support deploying assets separate from html #12128

Merged
merged 70 commits into from
May 2, 2019

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Feb 27, 2019

Description

If at first you don't succeed, try, try again 😅

The first attempt at asset prefixing went OK, but I think there was a fair enough amount of complexity that trying something new and hopefully simpler is valuable. Shout out to @pieh for the simpler idea here.

This PR introduces the ability to add assetPrefix to your gatsby-config.js. This should be a URL, and it is on the user of this functionality to ensure that assets are copied appropriately to this destination. It works with pathPrefix, as well, but again, slightly manual--for now!

As someone using this feature, here's how you would use it:

  • Add an assetPrefix option to your gatsby-config.js
    module.exports = {
      assetPrefix: `https://cdn.example.com`
    }
  • Continue to use functionality in your site that you've already been using, e.g. Link, withPrefix, etc. (no code or plugin changes should be required)
  • __PATH_PREFIX__ will be the joined result of assetPrefix and pathPrefix

Alright, alright. Now let's build this.

At build time, you will:

  • Run gatsby build --prefix-paths (just like you did with pathPrefix) and this will be respected whether using either of pathPrefix or assetPrefix
    • If you're using pathPrefix, you'll need to copy the files in public to a nested directory (e.g. if using /blog, will need to create move /public/ to /public/blog and deploy with your hosting provider of choice)
  • The public directory is assets (HTML, CSS, JS, etc.) so this can be uploaded to wherever, e.g. a CDN
    • Feel free to exclude HTML, but otherwise everything in this folder should already be an asset

This will create a version of your site that has its assets (CSS, JS, images, etc.) prefixed with ${assetPrefix}${pathPrefix || ''}, so those assets will need to be deployed and available. If so--this works just fine!

See gatsby-asset-prefix.netlify.com and the Github Repo for an example of this feature in use.

To do

  • Add documentation for assetPrefix
  • Add more tests
    • Not sure best approach of testing this e2e, but it'd be super nice to have some e2e tests for this functionality
  • Fix relative path being used with asset prefix
  • Fix e2e test failures (they seem legitimate)
  • Hard fail (?) when gatsby-plugin-offline is used with this feature; I do not believe you can serve service workers from a separate domain
    • It may be possible to get this working with some custom Workbox stuff, but I think the service worker will still need to be served from the same domain
    • cc @davidbailey00 any insight here? Would highly appreciate it!
  • [] Figure out what plugins--especially third party--this could break (generally, pathPrefix is used for assets, but it is also used as a "base path", e.g. for links)

Related Issues

Fixes #2335, Fixes #8833 (need to validate this one--but pretty sure it does!)

@@ -65,6 +65,15 @@ module.exports = async program => {
}
return next()
})
app.use(function(req, res, next) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could probably use cors here 🤷‍♂️

@@ -1,10 +1,18 @@
const Joi = require(`joi`)

const stripTrailingSlash = chain => chain.replace(/(\w)\//, `$1`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a nice addition - rather than requiring documentation (e.g. don't add a trailing slash), this will just remove it for us.

It only impacts a string with content, e.g. / is left alone.

Note: this should've been fine
@DSchau
Copy link
Contributor Author

DSchau commented Feb 27, 2019

Will need to add one more thing: a helper to create publicPath (what this really is) and pass that as pathPrefix to plugins.

Done in ad04a43

@DSchau DSchau requested a review from a team February 27, 2019 17:56
@DSchau
Copy link
Contributor Author

DSchau commented May 1, 2019

Almost done with the e2e tests.

One test is failing for an unknown reason, so I need to fix/figure out why!

 1) Production pathPrefix navigation can go back:
     CypressError: cy.then() timed out after waiting '9999ms'.

Your callback function returned a promise which never resolved.

The callback function was:

win => {
    if (!win.___apiHandler) {
      win.___apiHandler = _apiHandler.default.bind(win);
    }

    return _apiHandler.waitForAPI.call(win, api).then(() => subject);
  }

@DSchau DSchau removed the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label May 2, 2019
@DSchau
Copy link
Contributor Author

DSchau commented May 2, 2019

Alrighty--should have finished up the last, outstanding bug and test failure! Will get this merged and shipped... today!

@DSchau DSchau changed the title feat(gatsby): add assetPrefix option feat(gatsby): add assetPrefix to support deploying assets separate from html May 2, 2019
@DSchau DSchau merged commit 8291044 into gatsbyjs:master May 2, 2019
@DSchau DSchau deleted the gatsby/asset-prefix-improved branch May 2, 2019 19:08
@DSchau
Copy link
Contributor Author

DSchau commented May 2, 2019

Published as gatsby@2.4.0. I bumped the minor version for all packages in this PR, as well.

  • gatsby@2.4.0
  • gatsby-link@2.1.0 (gatsby@2.4.0 depends upon this, don't need to install manually!)
  • gatsby-plugin-feed@2.2.0
  • gatsby-plugin-manifest@2.1.0
  • gatsby-plugin-offline@2.1.0
  • gatsby-plugin-sitemap@2.1.0

Soon enough, the documentation will be available at /docs/asset-prefix/ (waiting for the build to complete), but you can get a sneak peek here

Thanks everyone for all the help testing this and getting it 🚢'd! ❤️

@andrewstaker
Copy link
Contributor

Thanks @DSchau and all those who pitched in to make this happen. This feature has saved our team a lot of headaches. 🙌

@leonfs
Copy link

leonfs commented May 3, 2019

Thanks @DSchau - Fantastic work.

@vv13
Copy link

vv13 commented May 3, 2019

My version is gatsby@2.3.4, then I got an error:

  29 |
  30 | function withPrefix(path) {
> 31 |   return normalizePath(__BASE_PATH__ + "/" + path);
     | ^
  32 | }
  33 |
  34 | function withAssetPrefix(path) {


  WebpackError: ReferenceError: __BASE_PATH__ is not defined

It make me confused, but fixed in gatsby@2.4.0.

@szimek
Copy link
Contributor

szimek commented May 4, 2019

@DSchau Huuuge thanks for releasing this feature!

I found one small issue when using relative path. I'm using gatsby-source-contentful and gatsby-transformer-remark plugins and all links with relative paths in markdown are prefixed with value of assetPrefix. I guess it's because of --path-prefix option, causing gatsby-transformer-remark to add pathPrefix to all relative links, which probably isn't necessary when using assetPrefix with either relative path or domain.

For now I fixed it with the following local plugin:

// plugins/gatsby-remark-rewrite-links/index.js
const visit = require('unist-util-visit');

// pathPrefix has value of assetPrefix here
module.exports = ({ markdownAST, pathPrefix }) => {
  visit(markdownAST, 'link', (node) => {
    if (node.url.startsWith(pathPrefix)) {
      node.url = node.url.replace(pathPrefix, '');
    }
  });

  return markdownAST;
};

which I then pass to gatsby-transformer-remark in gatsby-config.js:

{
  resolve: 'gatsby-transformer-remark',
  options: {
    plugins: [
      'gatsby-remark-rewrite-links',
      ...
    ]
  }
}

@leonfs
Copy link

leonfs commented May 8, 2019

Hi @DSchau.

We moved to version 2.4.2, and we are getting the following exceptions on build:

Error: ./.cache/gatsby-browser-entry.js
  Module not found: Error: Can't resolve 'gatsby-link' in ...

Error: ./.cache/navigation.js
  Module not found: Error: Can't resolve 'gatsby-link' in ...

You've mentioned in #12128 (comment) that the gatsby-link package should not be installed manually. But those 2 files are in the .cache folder and node will try to resolve the package at the root node_modules folder, it won't be able to infer that it's within the gatsby package in node_modules.

I can make it work if I install the package but I was just wondering if this has been an oversight due to the test repo using the resolutions feature, it could have slept through without noticing.

Is anyone else experiencing this problem?

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.

Question: Putting Assets in a Different Folder Ability to deploy non-html files to separate domain