resolve next config being deleted problem

[ymchun]

Fixes #371

changed

const withPWA = require('next-pwa')

module.exports = withPWA({
  pwa: {
    dest: 'public'
  }
})

to

const withPWA = require('next-pwa')
const nextConfig = {}

module.exports = withPWA(nextConfig, {
  dest: 'public'
})

[ymchun]

/** @type {import('next').NextConfig} */
const nextConfig = withPWA({
  reactStrictMode: true
});

seems this part doesn't need to be wrapped with withPWA

[Lukasdotcom]

LGTM

[marcofranssen]

The withPWA function does only have one input parameter. How does this fix that?

So where does your second argument be handled?

module.exports = (nextConfig = {})

Check #384 for the real fix.

[ymchun]

therefore, I introduced a breaking change to take in the second argument

next-pwa/index.js

Line 15 in 0ad2ea7

module.exports = (nextConfig = {}, pwa = {}) => ({

[marcofranssen]

Ah, I missed that part. However do we really need that breaking change? I think I'm my PR I fixed it without a breaking change unless I missed something.

[ymchun]

@marcofranssen whether having a breaking change at this moment is the choice of the maintainers. I would like to point out that both of our MRs are fixing the issue but which one is better solution and will last for longer time.

as the comment here pointed out Next.js will validate its config and warn for unknown properties. I would believe the reason behind this is to avoid putting third-party config into Next.js config.

next-pwa/index.js

Line 337 in 9d28c3b

// Next 12.2.3+ logs errors if the `pwa` field is returned since it is not recognized

for your MR, you solve the config issue. My MR solve not only the config issue but also align to Next.js config convention.

P.S. Next.js config function from Sentry for your reference.
https://github.com/getsentry/sentry-javascript/blob/fbb44d5e1860840e65f36cf4811c31291d51e186/packages/nextjs/src/config/index.ts

[AleksandarDev]

If we want to introduce breaking change - can we change it to be like other plugins for nextjs? eg.

const withMDX = require('@next/mdx')({
  options: {
    remarkPlugins: [],
    rehypePlugins: [],
  },
})
module.exports = withMDX(nextConfig)

or

const withBundleAnalyzer = require('@next/bundle-analyzer')({
    enabled: process.env.ANALYZE === 'true',
});

This way configuration for each plugin is on its own.

With this PR we would get bad example version...

// BAD EXAMPLE

const withBundleAnalyzer = require('@next/bundle-analyzer')({
    enabled: process.env.ANALYZE === 'true',
});

const withPWA = require('next-pwa');

module.exports = withBundleAnalyzer(withPWA(withMDX(nextConfig), {
  dest: 'public' // I don't know which plugin this belongs :/
})));

Having everything at the top:

// GOOD EXAMPLE

const withBundleAnalyzer = require('@next/bundle-analyzer')({
    enabled: process.env.ANALYZE === 'true',
});
const withPWA = require('next-pwa')({
    dest: 'public'
});

module.exports = withBundleAnalyzer(withPWA(withMDX(nextConfig)));

[ymchun]

@AleksandarDev is the following also a good example?

const withPWA = require('next-pwa')

let nextConfig = {
  // your next config goes here
}

nextConfig = withPWA(nextConfig, {
  // next-pwa config goes here
})

const withBundleAnalyzer = require('@next/bundle-analyzer')({
    enabled: process.env.ANALYZE === 'true',
});

const withMDX = require('@next/mdx')({
  options: {
    remarkPlugins: [],
    rehypePlugins: [],
  },
})

nextConfig = withBundleAnalyzer(withMDX(nextConfig));
// for the line above, I have a concern if there are many libraries to use, this line becomes a very long nested function call

module.exports = nextConfig

[AleksandarDev]

@AleksandarDev is the following also a good example?

I personally don't like reassigning, that aside, why have 2 different ways of initializing plugin when we have official implementations as the reference?

[DavidSint]

Having implemented a fix in #368 to solve one problem, clearly I didn't expect these issues to arise. Perhaps some tests would be needed to ensure this could be detected earlier.

Having looked into the next.js changes, I think it is this specific PR that brings in the validation that has the warnings against the way this package has its configuration in the same place as the Next.js configuration.

As part of the comments of the PR, @ijjk says:

the pattern we recommend for plugins can be seen in our next-mdx package which doesn't require adding the plugin specific configs to the returned config itself

and they provide the link: https://github.com/vercel/next.js/blob/e955850dd4880430433043fad3d8c13ed43b089a/packages/next-mdx/index.js#L1

Based on these comments, I don't see why this package should go against their recommendation. If this package continues to roll its own way, Next.js could implement something else in future which would mean the function signature would need to change again. Therefore, the best thing this package can do to protect itself from future breaking changes is to copy the Next.js recommended way of writing a plugin API.

So in summary, I agree with @AleksandarDev that the best approach is to copy the Next.js recommended plugin API signature.

[marcofranssen]

Having implemented a fix in #368 to solve one problem, clearly I didn't expect these issues to arise. Perhaps some tests would be needed to ensure this could be detected earlier.

Having looked into the next.js changes, I think it is this specific PR that brings in the validation that has the warnings against the way this package has its configuration in the same place as the Next.js configuration.

As part of the comments of the PR, @ijjk says:

the pattern we recommend for plugins can be seen in our next-mdx package which doesn't require adding the plugin specific configs to the returned config itself

and they provide the link: vercel/next.js@e955850/packages/next-mdx/index.js#L1

Based on these comments, I don't see why this package should go against their recommendation. If this package continues to roll its own way, Next.js could implement something else in future which would mean the function signature would need to change again. Therefore, the best thing this package can do to protect itself from future breaking changes is to copy the Next.js recommended way of writing a plugin API.

So in summary, I agree with @AleksandarDev that the best approach is to copy the Next.js recommended plugin API signature.

Fully Agree.

However before introducing a breaking change I think it would be great if the plugin also has a fix for the current release #384. Might make sense to merge this fix and then work on a refactor to follow the best practices.

[kirkegaard]

Its almost impossible to see what this fixes as the PR is filled with format changes. Can we please move the format changes to a separate PR, if those changes are really needed?