twopluszero / next-images

Import images in Next.js (supports jpg, jpeg, svg, png and gif images)
MIT License
948 stars 67 forks source link

Support dynamic assetPrefix #12

Closed SBoudrias closed 5 years ago

SBoudrias commented 5 years ago

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 commented 5 years ago

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

SBoudrias commented 5 years ago

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

arefaslani commented 5 years ago

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

SBoudrias commented 5 years ago

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 commented 5 years ago

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

https://github.com/webpack/webpack/issues/2776#issuecomment-233104666 https://github.com/webpack/webpack/issues/2776#issuecomment-233208623

SBoudrias commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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