twopluszero / next-images

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

dynamicAssetPrefix does not work as expected #63

Closed megazazik closed 3 years ago

megazazik commented 3 years ago

I need to change CDN host after build when server starts. I try it with statically generated pages (using revalidate option of getStaticProps) and with dynamically generated pages. All urls change correct except urls to images which I import from js.

I have tried to use dynamicAssetPrefix. My next.config.js looks like this:

const withImages = require('next-images');
module.exports = withImages({
    assetPrefix: process.env.CDN,
    dynamicAssetPrefix: true,
    webpack(config, options) {
        return config;
    },
});

If I set CDN at build time then this CDN host still does not change when I change CDN env and start server.

And if I leave CDN env empty at build time then I get an error TypeError: Cannot read property 'nextImagesAssetPrefix' of undefined during build.

Is it a bug? or I do something wrong? What is a correct way to use dynamicAssetPrefix?

arefaslani commented 3 years ago

Hi @megazazik, could you give me an example component so that I can see how you want to use it?

megazazik commented 3 years ago

My component looks like this

import img from './img.jpg';

export default function Home() {
    return (
        <div>
            <img src={img} width={300} />
        </div>
    );
}
arefaslani commented 3 years ago

@megazazik Seems like it's but. Working on it ATM...

arefaslani commented 3 years ago

@peter-jozsa I think the solution we've added doesn't work. Could you also take a look? There's also another problem I found: https://github.com/twopluszero/next-images/blob/master/index.js#L37 I think we need to remove !nextConfig.assetPrefix condition from this line, else it'll never get executed. Because if dynamicAssetPrefix is false, it'll skip this block and if it's true, it should get it from the assetPrefix property. So I think there's no way that dynamicAssetPrefix is true and assetPrefix is empty. What do you think?

peter-jozsa commented 3 years ago

I need to change CDN host after build when server starts. I try it with statically generated pages (using revalidate option of getStaticProps) and with dynamically generated pages. All urls change correct except urls to images which I import from js.

I have tried to use dynamicAssetPrefix. My next.config.js looks like this:

const withImages = require('next-images');
module.exports = withImages({
  assetPrefix: process.env.CDN,
  dynamicAssetPrefix: true,
  webpack(config, options) {
      return config;
  },
});

If I set CDN at build time then this CDN host still does not change when I change CDN env and start server.

And if I leave CDN env empty at build time then I get an error TypeError: Cannot read property 'nextImagesAssetPrefix' of undefined during build.

Is it a bug? or I do something wrong? What is a correct way to use dynamicAssetPrefix?

@megazazik You should leave assetPrefix empty during build time and dynamicAssetPrefix should be true. During runtime both dynamicAssetPrefix and assetPrefix should be set: assetPrefix must be the URL which will be prepended to image URLs. You should not get errors like that during build time. Could you share with us exactly which version of next and webpack are you using? Could you also tell us how you build your project and by build I mean which commands you use? Maybe the issue is with statically generated pages, I have never tested this feature with those.

@peter-jozsa I think the solution we've added doesn't work. Could you also take a look? There's also another problem I found: https://github.com/twopluszero/next-images/blob/master/index.js#L37 I think we need to remove !nextConfig.assetPrefix condition from this line, else it'll never get executed. Because if dynamicAssetPrefix is false, it'll skip this block and if it's true, it should get it from the assetPrefix property. So I think there's no way that dynamicAssetPrefix is true and assetPrefix is empty. What do you think?

Actually you might be right: we really don't need that negation in that condition because it only makes configuration more tricky if someone wants to use dynamic asset prefixes You can read above in my reply to @megazazik how you need to play with these two fields in order to get the feature working. But if we remove it from the mentioned condition we need to add another in this line in order to disable "static" assetPrefix substitution during build time if someone has dynamicAssetPrefix set to true in their next.config.js. I will open a PR soon and we can continue discussion of this change there.

arefaslani commented 3 years ago

@peter-jozsa

I will open a PR soon and we can continue discussion of this change there.

Ah that would be great! and thanks for the fast response 🙏

peter-jozsa commented 3 years ago

I created the PR: #64

megazazik commented 3 years ago

@peter-jozsa I have just created a new project via npx create-next-app. So there is the latest version of next. I did not make any config changes except adding next-images. And I use next build command for build. And yes, the error occurs only when I use statically generated pages.

peter-jozsa commented 3 years ago

@megazazik thanks for your quick response and sharing these information with us. I will look into it today and hopefully I will be able to come up with a solution/fix.

peter-jozsa commented 3 years ago

@megazazik I found the cause of your problem: the main reason why your build fails is because public runtime configuration is not available in statically generated pages. The next.js docs says it here

A page that relies on publicRuntimeConfig must use getInitialProps to opt-out of Automatic Static Optimization. Runtime configuration won't be available to any page (or component in a page) without getInitialProps.

Unfortunately we don't have any workaround for this issue at the moment.

@arefaslani we should make this clear in the README when we mention dynamic asset prefix

arefaslani commented 3 years ago

@peter-jozsa Yes, I agree.

megazazik commented 3 years ago

@peter-jozsa thanks for clarifying!

We really can't use publicRuntimeConfig at statically generated pages but we can use serverRuntimeConfig at any pages during SSR. And inside a browser the __webpack_public_path__ variable is available. Next.js sets it at the client https://github.com/vercel/next.js/blob/master/packages/next/client/index.tsx#L73

So dynamicAssetPrefix can work everywhere with this code.

if (isServer) {
  return `(require("next/config").default().serverRuntimeConfig.nextImagesAssetPrefix || '') + ${p}`;
}

return `(__webpack_public_path__ || '') + ${p}`;

But there is one more thing. Next.js adds /_next/ to assetPrefix at the client when it sets __webpack_public_path__. So we need to add /_next/ to the publicPath only at the server or we get duplicate /_next/ at the client in a url.

I have tried to test it with as many case as possible. I have not found any problems.

megazazik commented 3 years ago

I created a PR. https://github.com/twopluszero/next-images/pull/65

arefaslani commented 3 years ago

@peter-jozsa @megazazik Thank you both for creating PRs and improving the package. Let me check the changes this afternoon. I will talk to you soon 👍

peter-jozsa commented 3 years ago

@peter-jozsa thanks for clarifying!

We really can't use publicRuntimeConfig at statically generated pages but we can use serverRuntimeConfig at any pages during SSR. And inside a browser the __webpack_public_path__ variable is available. Next.js sets it at the client https://github.com/vercel/next.js/blob/master/packages/next/client/index.tsx#L73

So dynamicAssetPrefix can work everywhere with this code.

if (isServer) {
  return `(require("next/config").default().serverRuntimeConfig.nextImagesAssetPrefix || '') + ${p}`;
}

return `(__webpack_public_path__ || '') + ${p}`;

But there is one more thing. Next.js adds /_next/ to assetPrefix at the client when it sets __webpack_public_path__. So we need to add /_next/ to the publicPath only at the server or we get duplicate /_next/ at the client in a url.

I have tried to test it with as many case as possible. I have not found any problems.

@megazazik Wow nice, I checked your PR I think it makes sense and would solve this issue for sure. Thanks for your contribution, I have never used statically generated pages of Next.js good to have one here who has experience with it :)