Example showing that the C++ part is not ready for it:

'use strict';

const vm = require('vm');

let script = new vm.Script('var x = 0;', { produceCachedData: true });
const cachedData = script.cachedData;

new vm.Script('var x = 0;', { cachedData: new Uint16Array(cachedData.buffer) } );

$ ./node test.js
./node[10954]: ../src/node_contextify.cc:685:static void node::contextify::ContextifyScript::New(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[4]->IsUint8Array()' failed.
 1: 0x8dbd90 node::Abort() [./node]
 2: 0x8dbe65  [./node]
 3: 0x910fea node::contextify::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) [./node]
 4: 0xb6e1f1  [./node]
 5: 0xb6e7bf  [./node]
 6: 0x1c3b281cfc3d 
[1]    10954 abort (core dumped)  ./node test.js

Thank you, @ targos / ping reviewer(s).

Learned about the “vm” module & dug deeper in the node_contextify.cc code, based on the stack-trace error from your helpful test case, and able to use ArrayBufferView/IsArrayBufferView in places of Uint8Array/IsUint8Array. Please review at your convenience,

Tested w. the following test snippet:

‘use strict';

const vm = require('vm');

let script = new vm.Script('var x = 0;', { produceCachedData: true });
const cachedData = script.cachedData;

new vm.Script('var x = 0;', { cachedData: new Uint16Array(cachedData.buffer) } )

Ping/Dear reviewers,

Friendly reminder that - Made updates according per previous review comments, & was able to passed the suggested “vm” test w. cached data. Please review again at convenience.

@lundibundi,

I think change to the doc is missing here https://github.com/nodejs/node/blob/master/doc/api/vm.md#new-vmscriptcode-options and here https://github.com/nodejs/node/blob/master/doc/api/vm.md#vmcompilefunctioncode-params-options.

Updated the "cachedData" description for new-vmscriptcode-options & vmcompilefunctioncode-params-options docs. Thank you!

Also, I don't see any tests for TypedArray or DataView as cachedData, is there any?

The updated test in test/parallel/test-vm-cached-data.js for ERR_INVALID_ARG_TYPE should cover cachedData with the validation of TypedArray & DataView type error.

Please guide me other test(s) that'd add test for TypedArray or DataView as cachedData?

{
  code: 'ERR_INVALID_ARG_TYPE',
  type: TypeError,
-message: /must be one of type Buffer or Uint8Array/
+message: /must be one of type Buffer, TypedArray, or DataView/
}

Thanks for the swift changes =). It seems there is no reasonable way to test for cachedData besides simple arg checks, so it should be fine.

Though, I cannot understand how can we actually get the cachedData in anything besides Buffer. IIRC https://nodejs.org/api/vm.html#vm_script_createcacheddata is the only way to get cachedData therefore is there any sense of having cachedData available as TypedArray or DataView?

Also, this line https://github.com/nodejs/node/pull/22921/files#diff-0cf206672499c2f86db4ffb0cc5b668bR683 and this https://github.com/nodejs/node/pull/22921/files#diff-0cf206672499c2f86db4ffb0cc5b668bR1019 extracts data as uint8_t will this still be correct if we pass i.e. Int32Array (it should work but would that be correct)?

@lundibundi and/or other dear reviewers,

Per these two topics, I'd need some guideline to know how to update the PR to move forward. Would you mind advising?

...sense of having cachedData available as TypedArray or DataView"

...extracts data as uint8_t will this still be correct if we pass i.e. Int32Array (it should work but would that be correct)?

Ping / dear reviewers,

Would you mind guiding this PR for next step based on the questions in last comment? (Hoping the PR would not become stale; thank you!)

ping @addaleax @jasnell @targos as the ones who reviewed this could you provide some input on #22921 (comment)?

Just to be clear: I'm not blocking this, my concern is - maybe it's worth changing something here for it to become a bit more relevant.

CI: https://ci.nodejs.org/job/node-test-pull-request/17668/

I don’t quite see any tests for the newly supported types. You could use the common.getArrayBufferViews() function to make sure all TypedArray and DataView types are actually supported. Check out https://github.com/nodejs/node/blob/master/test/parallel/test-fs-write-file-typedarrays.js for an example

Ping / dear reviewers,

Per the suggestion in last comment, I've rebased & updated the tests in "test-vm-cached-data.js" to use common.getArrayBufferViews() function, to ensure all TypedArray and DataView types are actually supported. Please review again at convenience.

(P.S.: sorry about the delayed update. Was in Node+JS Interactive (glad to meet some of you in person), & a busy week at work after the conf. Looking forward to your review again to pick back up this PR, to work towards landing/closure. 🙏 )

CI: https://ci.nodejs.org/job/node-test-pull-request/18070/

Also, ping @targos, I believe your concerns were addressed.

@lundibundi, updated to use cachedData, since the term is indeed more contextually aligned than parsedData. Please review again at convenience. Thank you!

(P.S: also thank you @targos for reviewing the "native layer" part!)

Dear reviewers, (cc @refack & @lundibundi)

Thanks for validating for the "author ready" status & label & the CI run. Would you mind guiding to landing at your convenience?

If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md

P.S. If you have any question you can also feel free to contact me directly.

[that is my canned response to new contributors]
tl;dr A Collaborator (e.g. me) needs to makes sure all the items on the checklist are done, and land. I've triggered a new CI job https://ci.nodejs.org/job/node-test-pull-request/18280/. I will try to keep an eye on it, and land when it passes.

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18285/

Resume CI again: https://ci.nodejs.org/job/node-test-pull-request/18289/

Landed in 65fe999 🎉