Move config into an internal object instead of piggy backing on next config

[KidkArolis]

This is to avoid triggering the experimental feature warning:

[jnv]

Hi Karolis, I'd like to take a stab at this issue between holidays, do you have any preferences regarding the configuration placement?

i was thinking either to wrap plugin's configuration into nextImg property like this:

// next.config.js

module.exports = nextImg({
  nextImg: {
    breakpoints: [768],
  }
})

Alternatively, the configuration could be placed in a separate file, e.g. next-img.config.js. This is for example what next-optimized-images have planned for their next version.

Also should I consider backward compatibility? E.g. if the plugin finds a configuration key in top-level of config object, it should show a warning (using process.emitWarning) but keep using the configuration until the next major version of the package.

[KidkArolis]

Hi! That'd be awesome. I would avoid using a separate config file (seems overkill for a plugin configuration). Backwards compatibility is ok to break (we'll bump the version up), because I'd prefer if everyone moved to the new configuration, rather than having to maintain multiple approaches.

I think what you suggested should work, keep the plugin config in nextImg and not set that on the returned nextConfig, so that Next.js does not log any warnings. It's a shame Next plugins and their composability are not very well defined / thought through. For example, the official sass plugin keeps config directly on the root object: https://github.com/vercel/next-plugins/blob/master/packages/next-sass/index.js#L4 - that means that plugin also triggers "experimental features" warning incorrectly?

[jnv]

Well, looking at Next's source it seems the warning should be triggered only by the experimental property:

https://github.com/vercel/next.js/blob/72fae8bed45760e2f8c13e6e759a149156f4b5d7/packages/next/next-server/server/config.ts#L117

Is it perhaps possible this behavior changed between versions?

Anyway I think it's still a good practice to separate the plugin's configuration into an explicit namespace to prevent any possible collisions with other plugins or even Next's configuration.

[KidkArolis]

I tried upgrading to the latest version just now and I get that warning:

Could it be that that warning is triggered by next-compose-plugins and not next-img?

[KidkArolis]

Reported here, but the pkg author said they couldn't reproduce, hm.. cyrilwanner/next-compose-plugins#21

[jnv]

Okay, I have figured out why this happens; while the compose plugin plays part, as I explain here: cyrilwanner/next-compose-plugins#21 (comment), the main issue with the use of deepmerge here:

next-img/lib/next-plugin/nextPlugin.js

Line 50 in 384581f

pluginConfig = deepmerge.all([{}, defaults, pluginConfig], { arrayMerge: overwriteMerge })

By default deepmerge creates a deep clone of objects, therefore the experimental object passed from the default Next's config is lost. So basically if I add the deprecated clone: false option to deepmerge's call, it will solve the problem:

pluginConfig = deepmerge.all([{}, defaults, pluginConfig], { arrayMerge: overwriteMerge, clone: false }) 

However I think moving the plugin options to a separate property is still the cleanest solution.

[KidkArolis]

Thanks for looking into this!

Maybe we should switch to lodash's defaultsDeep, clearer intention and should not mutate keys that are not relevant:

const pluginConfig = defaultsDeep(pluginConfig, defaults)

Regarding putting the plugin settings into a namespace - what do other plugins do? My impression was that most "official" plugins set the top level config values. What does next-optimized-images or other 3 popular plugins do? We should try to do what everyone else is doing, unless everyone is doing it differently..

[jnv]

Regarding putting the plugin settings into a namespace - what do other plugins do? My impression was that most "official" plugins set the top level config values. What does next-optimized-images or other 3 popular plugins do? We should try to do what everyone else is doing, unless everyone is doing it differently..

You are right that most plugins, including next-optimized-images or next-mdx-blog keep the options in top-level object. However, next-optimized-images canary introduced a separate configuration file images.config.js (which makes sense since the plugin has been decoupled from Next.js into separate Babel and Webpack plugins).

My only concern is the options' names used by this plugin are rather generic enough to cause possible collision with other plugins or future Next.js' options. But I can see it's preferable to keep config compatibility for the time being rather than to unnecessarily future-proof it for now.

I will look into defaultsDeep.

[KidkArolis]

When I created this issue, ideally I was hoping it would be possible to keep the config of this plugin in a closure / internally without ever polluting the next object. So you pass the config with top level keys in the withImg call, but it never gets merged into the next config. I’m just not sure that’s possible if making it still compatible with next compose. Unless maybe its a double call thing withImg(config)().

…

On Mon, 28 Dec 2020 at 14:12, Jan Vlnas ***@***.***> wrote: Regarding putting the plugin settings into a namespace - what do other plugins do? My impression was that most "official" plugins set the top level config values. What does next-optimized-images or other 3 popular plugins do? We should try to do what everyone else is doing, unless everyone is doing it differently.. You are right that most plugins, including next-optimized-images or next-mdx-blog <https://github.com/hipstersmoothie/next-mdx-blog#configuration> keep the options in top-level object. However, next-optimized-images canary introduced a separate configuration file images.config.js <https://github.com/cyrilwanner/next-optimized-images/tree/canary#imagesconfigjs> (which makes sense since the plugin has been decoupled from Next.js into separate Babel and Webpack plugins). My only concern is the options' names used by this plugin are rather generic enough to cause possible collision with other plugins or future Next.js' options. But I can see it's preferable to keep config compatibility for the time being rather than to unnecessarily future-proof it for now. I will look into defaultsDeep. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPGWGAZVN3XMVNKF3PFVTSXCG5TANCNFSM4N6JM7PQ> .

[jnv]

Okay, so I got a bit tangled into this. The problem is that defaultsDeep concatenates arrays by default, which isn't desirable nor consistent with current behavior. The recommendation from lodash maintainers is to use mergeWith – but that one creates a new object reference so we are back to square one.

There is discussion in TehShrike/deepmerge#212 to un-deprecate clone option in deepmerge, so I think it may be easiest to keep on using that library.

ideally I was hoping it would be possible to keep the config of this plugin in a closure / internally without ever polluting the next object.

Yeah, I also though whether that could be a way out. I think it would be possible like this, but it would add alternative behavior in case the compose plugins is not used. Basically:

• withImg checks if nextComposePlugins is present (or specifically, if phase is present)
• If yes, return just { webpack } function; compose plugins will take care of shallowly merging the updated config with original config.
• If no return the whole merged object.