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

test: remove outdated V8 flag #895

Closed
wants to merge 2 commits into from

Conversation

RaisinTen
Copy link
Contributor

The --concurrent-array-buffer-freeing flag is going to be removed
upstream in V8 9.0.

Refs: v8/v8@0a480c3

The `--concurrent-array-buffer-freeing` flag is going to be removed
upstream in V8 9.0.

Refs: v8/v8@0a480c3
@RaisinTen
Copy link
Contributor Author

cc @mhdawson could you please take a look once you are free? :)

@@ -4,7 +4,7 @@ const majorNodeVersion = process.versions.node.split('.')[0];

if (typeof global.gc !== 'function') {
// Construct the correct (version-dependent) command-line args.
let args = ['--expose-gc', '--no-concurrent-array-buffer-freeing'];
let args = ['--expose-gc'];
Copy link
Member

Choose a reason for hiding this comment

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

Did you check the history of why this was added in the first place. I'm wondering if adding it needs to be Node.js version dependent versus just removing completely (ie might results in failures on older Node.js versions...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't check it before making this PR. I checked now and the PR that introduced this is: #737. It fixed this issue: #735. I'm not sure whether removing this flag would end up causing the failure again because we don't have any Windows tests in the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check to conditionally add the option if v8 has a major version that is less than 9. PTAL.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for updating.

mhdawson pushed a commit that referenced this pull request Feb 12, 2021
The `--concurrent-array-buffer-freeing` flag is going to be removed
upstream in V8 9.0.

PR-URL: #895
Refs: v8/v8@0a480c3
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed as f7ed249 thanks @RaisinTen

@mhdawson mhdawson closed this Feb 12, 2021
@RaisinTen RaisinTen deleted the test/remove-outdated-V8-flag branch February 13, 2021 15:43
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
The `--concurrent-array-buffer-freeing` flag is going to be removed
upstream in V8 9.0.

PR-URL: nodejs/node-addon-api#895
Refs: v8/v8@0a480c3
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
The `--concurrent-array-buffer-freeing` flag is going to be removed
upstream in V8 9.0.

PR-URL: nodejs/node-addon-api#895
Refs: v8/v8@0a480c3
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
The `--concurrent-array-buffer-freeing` flag is going to be removed
upstream in V8 9.0.

PR-URL: nodejs/node-addon-api#895
Refs: v8/v8@0a480c3
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
The `--concurrent-array-buffer-freeing` flag is going to be removed
upstream in V8 9.0.

PR-URL: nodejs/node-addon-api#895
Refs: v8/v8@0a480c3
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants