Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

fix: don't fail if critters is missing on Next.js>=10.0.4 #295

Merged
merged 2 commits into from
Jan 11, 2021

Conversation

erezrokah
Copy link
Contributor

- Summary

Related to netlify/next-on-netlify#103 (comment)
Doesn't fix the original issue as the original issue is about a missing react-native module (we don't have a reproduction for it).

This is a result of a recent change in Next.js

Note: This use case is not covered by the logic that makes

await zipNode(t, 'node-module-deep-conditional')
pass, since we tree shake Next.js dependencies instead of copying published files, side files and nested modules.

A better solution would be to support configuring external modules (see #68) so the Next.js plugin (or any other plugin) can generate a relevant bundling configuration based on the framework.
That bundling configuration can change based on the site configuration, for example if the user has optimizeCss configured for their Next.js site, we would like to bundle the critter dependency.

Regardless, this is an issue with Next.js as critters should be an optional dependency.

- Test plan

Added unit tests

- Description for the changelog

Don't fail when critters module is missing on Next.js>=10.0.4

- A picture of a cute animal (not mandatory but encouraged)
image

@erezrokah erezrokah added the type: bug code to address defects in shipped code label Jan 11, 2021
@@ -60,4 +65,17 @@ const isOptionalModule = function (
)
}

// 'critters' is used only in Next.js >= 10.0.4 when enabling an experimental option and has to be installed manually
// we ignore it if it's missing
const isExternalModule = function (moduleName, { dependencies = {}, devDependencies = {} }) {
Copy link
Contributor

@ehmicky ehmicky Jan 11, 2021

Choose a reason for hiding this comment

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

Should we abstract this by taking the module names and versions from some constant variable such as:

const EXTERNAL_MODULES = {
  critters: { next: '>= 10.0.4' }
}

This would allow adding more of those.

Alternatively we could also wait for another case of external module to abstract, but then maybe we should rename isExternalModule() to isExternalCrittersModule()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively we could also wait for another case of external module to abstract

Let's wait for another use case.
Not sure we want this to be a long term fix anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants