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

Next.js version support #25

Closed
ehmicky opened this issue Nov 10, 2020 · 7 comments · Fixed by #96 or netlify/next-on-netlify#167
Closed

Next.js version support #25

ehmicky opened this issue Nov 10, 2020 · 7 comments · Fixed by #96 or netlify/next-on-netlify#167
Assignees
Labels
proj/non/next-on-netlify-enterprise type: bug code to address defects in shipped code

Comments

@ehmicky
Copy link

ehmicky commented Nov 10, 2020

next-on-netlify requires users to install next themselves, i.e. it is a peerDependency.

https://github.com/netlify/next-on-netlify/blob/a48735f625cc7ad1a080b322673d3e9a11f42d27/package.json#L37-L39

Furthermore, it requires them to install a specific Next.js version (v9 even though v10 is the latest).

In this plugin, we are using the next package to load the configuration file

https://github.com/netlify/netlify-plugin-nextjs/blob/2849dc5f7c57e9fd827a939e067a871e4cb487b1/index.js#L7

https://github.com/netlify/netlify-plugin-nextjs/blob/2849dc5f7c57e9fd827a939e067a871e4cb487b1/package.json#L30

We might be missing a clear strategy on Next.js versioning compatibility:

  • When a new release of Next.js is made (e.g. Next 10), users of that new release would not be able to use it on both next-on-netlify and this plugin, because both are pinning a specific Next.js major version
  • Users with older Next.js versions (e.g. Next 8) might not be able to use both next-on-netlify and this plugin

As new versions of Next.js will be released, the lack of a clear strategy might become a bigger problem.

I would recommend the following:

  • Enforcing a minimum Next.js version with this plugin
  • Using a looser >= range for both this plugin and next-on-netlify in the peerDependency version range, so that we can support multiple Next.js major releases
  • Using the user's Next.js version to load the configuration file, i.e. making next a peerDependencies instead of a dependencies in this plugin's package.json
  • Ensure Next 10 is supported
  • When time comes to make this plugin opt-out instead of opt-in for all Netlify users, builds should check the site's Next.js version to ensure it is recent enough to use this plugin

What do you think?

@ehmicky
Copy link
Author

ehmicky commented Nov 16, 2020

Based on discussions with @lindsaylevine and @cassidoo on Slack, our current solution would be to only support Next.js ^9.5.3 users.
This excludes Next.js 8 users and Next.js <9.5.3 users.
This also excludes Next.js 10 users for the moment, due to the current unstability of Next.js 10. However, we should do a follow-up to support that major version as soon as possible.

Enabling the plugin by default in new sites should only happen if the site is using one of our officially supported Next.js versions. We could allow Next.js 10 users to use this plugin providing they explicitly install it though.

This would result in the following actionable items:

  • In this plugin, add "next": ">=9.5.3" as a peerDependencies instead of dependencies
  • In next-on-netlify, change the peerDependencies version range more precise: >=9.5.3 instead of 9.x
  • In onPreBuild() of this plugin, if next is not available, fail the build with an error message (issue at Plugin load if next is not available #8)
  • In onPreBuild() of this plugin, if next is <9.5.3, fail the build with an error message
  • In onPreBuild() of this plugin, if next is >=10.0.0, do not fail the build but print a warning message that support is experimental
  • When time comes to make this plugin opt-out instead of opt-in for all Netlify users, builds should check the site's Next.js version to ensure it is ^9.5.3 (not >=9.5.3) to use this plugin

We should then add two additional GitHub issues for:

  • Supporting Next.js 10 (see issue)
  • Devising a strategy for forward compatibility the next time we'll need to change the range of supported Next.js versions.

What do you think?

This was referenced Nov 16, 2020
@ehmicky
Copy link
Author

ehmicky commented Nov 17, 2020

Several items done at #45, netlify/next-on-netlify#90 and #46

@ehmicky
Copy link
Author

ehmicky commented Nov 19, 2020

next had to reverted from a peer dependency to a production dependency due to #55.
Once https://github.com/netlify/pod-the-builder/issues/102 is done, we should make it a peer dependency again so that users can choose their version of Next.js.

@ehmicky
Copy link
Author

ehmicky commented Nov 23, 2020

For the forward compatibility, one possible approach could be to lock users with older Next.js versions to an older version of this plugin. How this is implemented would depend on the Build plugins architecture by the time we need this, so I would suggest waiting until we need to drop support for older Next.js versions before looking into this.

@ehmicky
Copy link
Author

ehmicky commented Dec 14, 2020

PR at #73 to re-enable next as a peer dependency.

@ehmicky
Copy link
Author

ehmicky commented Feb 8, 2021

npm 7 just became public and changes how peerDependencies are installed. With the new behavior:

  • If the site has next >= 9.5.3 as a dependency, next-on-netlify will not re-install it nor install a different version.
  • If the site has next < 9.5.3 as a dependency, next-on-netlify will fail during npm install.
  • If the site does not have next as a dependency, next-on-netlify will install it. We currently use >=9.5.3, so it would install the latest version.

We have two choices:

  • If we keep next as a peerDependencies, sites which did not install next will not fail. Instead, they will use the latest version of next by default. This means any major releases of next might break their site. This also mean we can remove the check for "is next installed?" since next would always be installed.
  • If we remove next from peerDependencies, sites which did not install next will fail. However, sites which installed an unsupported version of next (for example Next 9.0.0) will not fail during npm install. Thanks to the following change, this should be ok though.

I would favor the second choice, i.e. we should remove the peerDependencies from both next-on-netlify and this repository. Thoughts?

@erezrokah
Copy link

I would favor the second choice, i.e. we should remove the peerDependencies from both next-on-netlify and this repository. Thoughts?

Agree the second one makes the most sense, also due to this https://github.com/netlify/netlify-plugin-nextjs/blob/9b7b23e33167e4c295dd8124c96672f544d820c0/index.js#L80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proj/non/next-on-netlify-enterprise type: bug code to address defects in shipped code
Projects
None yet
2 participants