fix: node18 support for netlify/build #4726
Conversation
|
This pull request adds or modifies JavaScript ( |
| @@ -37,15 +37,14 @@ const tStartPlugins = async function ({ pluginsOptions, buildDir, childEnv, logs | |||
| export const startPlugins = measureDuration(tStartPlugins, 'start_plugins') | |||
|
|
|||
| const startPlugin = async function ({ pluginDir, nodePath, buildDir, childEnv }) { | |||
| const childProcess = execaNode(CHILD_MAIN_FILE, { | |||
| const childProcess = execaNode(CHILD_MAIN_FILE, [], { | |||
There was a problem hiding this comment.
This are the types of execaNode seems like we where using it wrongly:
2nd argument here are the arguments passed down to the child process
| @@ -2788,14 +2788,10 @@ Generated by [AVA](https://avajs.dev). | |||
| CACHE_DIR: '.netlify/cache',␊ | |||
| IS_LOCAL: true,␊ | |||
| NETLIFY_BUILD_VERSION: '1.0.0',␊ | |||
| SITE_ID: undefined,␊ | |||
There was a problem hiding this comment.
JSON serialisation contains undefined values in comparison of v8.serialise. which should not make any difference just the snapshot looks differently.
There was a problem hiding this comment.
I think you mean the other way around, i.e. JSON serialisation omits undefined values?
|
Reading about this seems that the serialization is backwards compatible (node 18 can deserialize data from older node versions) but not forward compatible (older node cannot deserialize data from node 18) |
| @@ -2788,14 +2788,10 @@ Generated by [AVA](https://avajs.dev). | |||
| CACHE_DIR: '.netlify/cache',␊ | |||
| IS_LOCAL: true,␊ | |||
| NETLIFY_BUILD_VERSION: '1.0.0',␊ | |||
| SITE_ID: undefined,␊ | |||
There was a problem hiding this comment.
I think you mean the other way around, i.e. JSON serialisation omits undefined values?
|
I'm not planning to roll this out in our low risk week but after it I would do a soft roll out via canary? |
|
Sounds good! 🚀 |
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes https://github.com/netlify/pillar-workflow/issues/624
Running on Node 16.x
netlify-build --nodePath /Users/lukasholzer/.nvm/versions/node/v18.11.0/bin/nodeis working now with this change on a next.js site.This basically reverts ded368d#diff-aadec7c55ab8f716f3c2d33ec17ce26331572cf6f1eada1bc9274b4363ebbc80
IMHO a IPC communication always needs to be serialisable as JSON otherwise we will always run in problems with different node major versions.
I'm currently testing if something is not serialisable inside the errors but until now I could. not find any issue (as mikael pointed out back in the commit that introduced the
advancedserialization). as we still to the error to json conversion: https://github.com/netlify/build/blob/main/packages/build/src/error/build.js#L8-L17For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)
