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

Make configuration of dynamic asset prefix easier #64

Conversation

peter-jozsa
Copy link
Contributor

This PR tries to make the usage of Dynamic Asset Prefix feature more clear. From now on if dynamicAssetPrefix field in next.config.js is set to true we will always produce a dynamic url (based on the assetPrefix extracted from Next.js runtime config) for imported images during transpilation. Unlike before we do not check during transpilation if assetPrefix field is defined or not in the next.config.js.

index.js Outdated
@@ -31,10 +31,10 @@ module.exports = ({ dynamicAssetPrefix = false, ...nextConfig } = {}) => {
options: {
limit: nextConfig.inlineImageLimit,
fallback: require.resolve("file-loader"),
publicPath: `${nextConfig.assetPrefix || nextConfig.basePath}/_next/static/images/`,
publicPath: `${(dynamicAssetPrefix ? '' : nextConfig.assetPrefix) || nextConfig.basePath}/_next/static/images/`,
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this nextConfig.basePath from this line. Because I think basePath is for links, not for assets which we're trying to solve with this package. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no static asset prefix is provided and dynamic asset prefix is disabled we should still fallback to basePath becuase assets will be served under the basePath if it is specified. But if assetPrefix is set or dynamicAssetPrefix is truish then we don't have to take basePath into consideration: in this case static/dynamic assetPrefix should contain the basePath part too. I fix the condition here to represent this.

@peter-jozsa
Copy link
Contributor Author

I've also added the warning message to README about the limitations of dynamic asset prefixes and statically generated pages.

@peter-jozsa
Copy link
Contributor Author

I'm closing it since the PR #65 of @megazazik already contains this change plus he took care of the limitations related to statically generated pages.

@peter-jozsa peter-jozsa deleted the feature/dynamic-asset-prefix-configuration branch January 19, 2021 08:24
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.

2 participants