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

Support dynamic assetPrefix #12

Closed
SBoudrias opened this issue Jan 28, 2019 · 14 comments
Closed

Support dynamic assetPrefix #12

SBoudrias opened this issue Jan 28, 2019 · 14 comments

Comments

@SBoudrias
Copy link
Contributor

Currently this plugin only works if assetPrefix is set at build step. (meaning it only support a static CDN)

This is quite hard to get working when considering apps running on multiple environments; where each environment could have a different CDN (like a production CDN vs a staging CDN.)

Furthermore, Next.js expose many ways to make the assetPrefix dynamic (like switching CDN domain based on the app domain, etc.) By requiring a static CDN, this plugin cannot support any of those use cases.

@arefaslani
Copy link
Member

@SBoudrias Sorry for late response. I'll work on that as soon as possible. But if you can help I'll appreciate that.

@SBoudrias
Copy link
Contributor Author

@arefaslani I gave this issue a try already, but I didn't find the right way. Any ideas?

@arefaslani
Copy link
Member

arefaslani commented Feb 1, 2019

@SBoudrias Currently I'm so busy. But be sure that I'll put the effort to find a solution soon.

@SBoudrias
Copy link
Contributor Author

Just to give some details, this here works when navigating on the client side:

module.exports = (nextConfig = {}) => {
  return Object.assign({}, nextConfig, {
    webpack(config, options) {
      const { isServer } = options;
      nextConfig = Object.assign({ inlineImageLimit: 8192 }, nextConfig);

      if (!options.defaultLoaders) {
        throw new Error(
          "This plugin is not compatible with Next.js versions below 5.0.0 https://err.sh/next-plugins/upgrade"
        );
      }

      config.module.rules.push({
        test: /\.(jpe?g|png|svg|gif|ico|webp)$/,
        use: [
          {
            loader: "url-loader",
            options: {
              limit: nextConfig.inlineImageLimit,
              fallback: "file-loader",
              outputPath: "static/images/",
              name: "[name]-[hash].[ext]"
            }
          }
        ]
      });

      if (typeof nextConfig.webpack === "function") {
        return nextConfig.webpack(config, options);
      }

      return config;
    }
  });
};

Just removing the publicPath means webpack will use the __webpack_public_path__ value; and Next.js sets it to assetPrefix automatically.

But this value doesn't exists when rendering on the SSR, which mean we then end up with the relative path, and that ends up being sent with the server rendered HTML; causing 404 on the images. I'm not sure how to workaround this issue.

@arefaslani
Copy link
Member

@SBoudrias Sorry for late reply. Can you check these comments to see if they help?

webpack/webpack#2776 (comment)
webpack/webpack#2776 (comment)

@SBoudrias
Copy link
Contributor Author

Interesting, but I don't think the issue is about hoisting here. Next.js just do not try to set this value on the server side.

@arefaslani
Copy link
Member

@SBoudrias I don't think that it's possible to use dynamic imports in Webpack configuration as in the Next app. Because Next sets the asset prefix in the app level but we can't do that when defining Webpack loaders. I think the best way for doing this is by using an env variable like process.env.ASSET_PREFIX and set the value of assetPrefix config in the Webpack file equal to this env variable. I mean something like

// next.config.js
const withImages = require('next-images')
module.exports = withImages({
  inlineImageLimit: 16384,
  assetPrefix: process.env.ASSET_PREFIX,
  webpack(config, options) {
    return config
  }
})

You can reopen this issue if you think this issue can be solved in a better way.

@eugef
Copy link

eugef commented Aug 14, 2019

I have exactly the same issue with SSR images not picking up the assetPrefix.
But on top of that I am dynamically change assetPrefix per request as described here https://github.com/zeit/next.js#dynamic-assetprefix

This means I cannot use previous solution, as ENV variable is not bounded to the request context and two simultaneous requests will override the same variable.

Do you have any suggestion how to approach this issue?

On a side note: it sounds weird to me that while Next.js uses the same Webpack config both for CSR and SSR the assetPrefix doesn't work only for SSR.

@arefaslani
Copy link
Member

I have exactly the same issue with SSR images not picking up the assetPrefix.
But on top of that I am dynamically change assetPrefix per request as described here https://github.com/zeit/next.js#dynamic-assetprefix

This means I cannot use previous solution, as ENV variable is not bounded to the request context and two simultaneous requests will override the same variable.

Do you have any suggestion how to approach this issue?

On a side note: it sounds weird to me that while Next.js uses the same Webpack config both for CSR and SSR the assetPrefix doesn't work only for SSR.

I should work on it in my holiday this week...

@maciejmajewski
Copy link

@eugef @arefaslani have you found a solution maybe? I can confirm that webpack uses correct assetPrefix when doing CSR, but doesn't with SSR.

@maciejmajewski
Copy link

maciejmajewski commented Nov 5, 2019

So FYI I was able to make it work with following config:

publicPath: `${isServer ? "/_next/" : ""}static/images/`,
outputPath: "static/images/",
postTransformPublicPath: (p) => {
  if (isServer) {
    return `(process.env.ASSET_PREFIX || '') + ${p}`;
  } else {
    return `__webpack_public_path__ + ${p}`
  }
},

postTransformPublicPath is available in file-loader from version 4.2.0: https://github.com/webpack-contrib/file-loader/blob/master/CHANGELOG.md#420-2019-08-07


Edit: to clarify, this sample helps set proper public path when SSR doesn't have proper value for __webpack_public_path__, it does not solve issues with having changing public path according to request.

@eugef
Copy link

eugef commented Nov 6, 2019

@maciejmajewski please correct me if I am wrong, but your solution still doesn't allow to have different asset prefix per request, as postTransformPublicPath doesn't have access to the request context.

@maciejmajewski
Copy link

@eugef Unfortunately this wasn't my use-case so I didn't have to go that far, my apologies for false hope. With my sample I have solved SSR not having proper value for __webpack_public_path__.

@SBoudrias
Copy link
Contributor Author

@maciejmajewski could you send a PR to this plugin so this part can be fixed?

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

No branches or pull requests

4 participants