Skip to content

fix: force serverless target #343

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 25, 2021
Merged

fix: force serverless target #343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8dc7ca1
fix: force serverless target
ascorbic May 24, 2021
5b71269
fix: changes from review
ascorbic May 25, 2021
a1ab7bf
fix: extract target checks
ascorbic May 25, 2021
630cb1a
fix: fix tests
ascorbic May 25, 2021
09642bd
fix: changes from review
ascorbic May 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion helpers/getNextConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict'

const { cwd: getCwd } = require('process')
const { resolve } = require('path')

const moize = require('moize')

Expand Down
23 changes: 20 additions & 3 deletions helpers/hasCorrectNextConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const getNextConfig = require('./getNextConfig')

// Checks if site has the correct next.config.js
const hasCorrectNextConfig = async ({ nextConfigPath, failBuild }) => {
// In the plugin's case, no config is valid because we'll make it ourselves
Expand All @@ -12,9 +11,27 @@ const hasCorrectNextConfig = async ({ nextConfigPath, failBuild }) => {
const isValidTarget = acceptableTargets.includes(target)
if (!isValidTarget) {
console.log(
`Your next.config.js must set the "target" property to one of: ${acceptableTargets.join(', ')}. Update the
target property to allow this plugin to run.`,
`The "target" config property must be one of "${acceptableTargets.join('", "')}". Setting it to "serverless".`,
Copy link

Choose a reason for hiding this comment

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

"Settingit to serverless" might imply to some that we're actually mutating/updating/changing their config file (which i don't think we're doing? 🤔 ). maybe "forcing the build to use target serverless" or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

)

/* eslint-disable fp/no-delete, node/no-unpublished-require */

// We emulate Vercel so that we can set target to serverless if needed
process.env.NOW_BUILDER = true
// If no valid target is set, we use an internal Next env var to force it
process.env.NEXT_PRIVATE_TARGET = 'serverless'
Copy link

Choose a reason for hiding this comment

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

I believe some automated tests might be using the same process, i.e. running one test would change process.env for other tests. We could probably fix this by either:

  • Spawning those tests in different processes
  • Loading next right away so it uses those environment variables, then deleting process.env.NOW_BUILDER and process.env.NEXT_PRIVATE_TARGET (or restoring them to their original value).


// 🐉 We need Next to recalculate "isZeitNow" var so we can set the target, but it's
// set as an import side effect so we need to clear the require cache first. 🐲
// https://github.com/vercel/next.js/blob/canary/packages/next/telemetry/ci-info.ts

delete require.cache[require.resolve('next/dist/telemetry/ci-info')]
delete require.cache[require.resolve('next/dist/next-server/server/config')]
Copy link

Choose a reason for hiding this comment

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

Considering next is performing some logic at require()-time, this approach makes sense.

The only alternative I can think of would be to call getNextConfig() in a separate process. However, this would add some significant complexity. Also, this would JSON-serialize the Next.js configuration, which would not work if the config includes non-JSON values (can it?), unless we use serialization: 'advanced' but that option is not available in Node 10.

Two potential problems we might have in the future though are:

  • I am unsure whether this would work with ES modules since the current Node.js implementation does not allow removing cached URLs (it allows passing query parameters as cache key, but this would not work here), so this might require mixing CommonJS require() and ES modules import in order to keep using require.cache.
  • This relies on next internal file structure. The cache deletion would be a noop if they choose to rename those files. Some things we could do to fix this:
    • Run some automated tests confirming those files exist with the latest version of next
    • delete all the next entries in require.cache instead of those specific files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we're already relying on the internal layout as we're using unpublished exports to get the config in the first place


// Clear memoized cache
getNextConfig.clear()
Copy link

Choose a reason for hiding this comment

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

why do we have to clear here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is memoized, so it will return the old value if we don't clear it

Copy link

Choose a reason for hiding this comment

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

This works.
In the long run, we probably should consider removing memoization on getNextConfig() and passing its return value around function calls instead.


/* eslint-enable fp/no-delete, node/no-unpublished-require */
}

return isValidTarget
Expand Down
17 changes: 2 additions & 15 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,8 @@ module.exports = {
if (hasNoPackageJson) {
return failBuild('Could not find a package.json for this project')
}

const pluginNotNeeded = await doesNotNeedPlugin({ netlifyConfig, packageJson, failBuild })

if (!pluginNotNeeded) {
const nextConfigPath = await findUp('next.config.js')
if (nextConfigPath === undefined) {
// Create the next config file with target set to serverless by default
const nextConfig = `
module.exports = {
target: 'serverless'
}
`
await pWriteFile('next.config.js', nextConfig)
}
}
// Populates the correct config if needed
await doesNotNeedPlugin({ netlifyConfig, packageJson, failBuild })

// Because we memoize nextConfig, we need to do this after the write file
Copy link

Choose a reason for hiding this comment

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

we can prob remove this comment now i think?

Copy link

Choose a reason for hiding this comment

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

double ping on this one?

Suggested change
// Because we memoize nextConfig, we need to do this after the write file

const nextConfig = await getNextConfig(utils.failBuild)
Expand Down