GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — 48198 node api external strings by gabrielschulhof · Pull Request #48339 · nodejs/node

Conversation

@gabrielschulhof

@gabrielschulhof gabrielschulhof commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 5, 2023
@gabrielschulhof gabrielschulhof changed the title 48198 node api external strings [wip] 48198 node api external strings Jun 5, 2023
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to Node-API. label Jun 5, 2023
gabrielschulhof
gabrielschulhof commented Jun 5, 2023
View reviewed changes
Comment thread src/js_native_api_v8.cc Outdated

@gabrielschulhof gabrielschulhof Jun 5, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Check with Electron if this is needed for strings.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gabrielschulhof
gabrielschulhof commented Jun 5, 2023
View reviewed changes
Comment thread src/js_native_api_v8.cc Outdated

@gabrielschulhof gabrielschulhof Jun 5, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is it OK if lengh is NAPI_AUTO_LENGTH?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gabrielschulhof gabrielschulhof Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Chose to assume that it is not, and used C++ string view to compute length.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daeyeon
daeyeon reviewed Jun 5, 2023
View reviewed changes
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
gabrielschulhof and others added 13 commits June 7, 2023 07:21
@gabrielschulhof
node-api: unify string creation
bd8045b
@gabrielschulhof
node-api: add string creation benchmark
50a2109
@gabrielschulhof
node-api: implement external strings
1364872
@gabrielschulhof
add test
a9e24b9
@gabrielschulhof
rename to node_api_*
e1127d1
@gabrielschulhof
put back build type
953304c
@gabrielschulhof
make linter work
09f7cdf
@gabrielschulhof
rename as requested
902cb33
@gabrielschulhof
use std::basic_string_view to calc length in case of NAPI_AUTO_LENGTH
754864e
@gabrielschulhof
test for auto-length
95a9971
@gabrielschulhof
format
b4f383b
@gabrielschulhof
correct length
05317ac
@gabrielschulhof
doc
20ab0fb
@gabrielschulhof
gabrielschulhof force-pushed the 48198-node-api-external-strings branch from cf6e959 to 20ab0fb Compare June 7, 2023 15:25
gabrielschulhof added 2 commits June 7, 2023 17:53
@gabrielschulhof
use right length indicator
ca4c08c
@gabrielschulhof
final adjustment
5de34d4
gabrielschulhof
gabrielschulhof commented Jun 8, 2023
View reviewed changes
Comment thread test/js-native-api/test_string/test_string.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/js-native-api/test_string/test_string.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/js-native-api/test_string/test_string.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof
Apply suggestions from code review
64f2498
@gabrielschulhof gabrielschulhof changed the title [wip] 48198 node api external strings 48198 node api external strings Jun 8, 2023
daeyeon
daeyeon reviewed Jun 8, 2023
View reviewed changes
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof @daeyeon
Apply suggestions from code review
cb9d95a 
Co-authored-by: Daeyeon Jeong <[email protected]>
gabrielschulhof
gabrielschulhof commented Jun 11, 2023
View reviewed changes
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/js_native_api_v8.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
gabrielschulhof and others added 3 commits June 11, 2023 15:35
@gabrielschulhof
Apply suggestions from code review
5c11c81
@gabrielschulhof
use common distructor
b651fca
@gabrielschulhof
format
668fd4d
daeyeon
daeyeon reviewed Jun 12, 2023
View reviewed changes
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
gabrielschulhof and others added 2 commits June 11, 2023 20:36
@gabrielschulhof @daeyeon
Apply suggestions from code review
c721439 
Co-authored-by: Daeyeon Jeong <[email protected]>
@gabrielschulhof
md lint
7f67a2f
daeyeon
daeyeon approved these changes Jun 12, 2023
View reviewed changes
@daeyeon daeyeon added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jun 12, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 12, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 12, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52220/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jun 12, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52224/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jun 13, 2023
CI Reliability 2023-06-13 nodejs/reliability#592
Open
CI Reliability 2023-06-14 nodejs/reliability#593
Open
gabrielschulhof added a commit that referenced this pull request Jun 14, 2023
@gabrielschulhof
node-api: implement external strings
60d9aed 
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
@gabrielschulhof

gabrielschulhof commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

Landed in 60d9aed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof closed this Jun 14, 2023
@toyobayashi toyobayashi mentioned this pull request Jun 14, 2023
external strings toyobayashi/emnapi#68
Merged
This was referenced Jun 15, 2023
CI Reliability 2023-06-15 nodejs/reliability#594
Open
CI Reliability 2023-06-21 nodejs/reliability#595
Open
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@gabrielschulhof @RafaelGSS
node-api: implement external strings
87ad657 
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
v20.4.0 proposal #48643
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@gabrielschulhof @Ceres6
node-api: implement external strings
0133a82 
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: nodejs#48339
Fixes: nodejs#48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@gabrielschulhof @Ceres6
node-api: implement external strings
98f97dc 
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: nodejs#48339
Fixes: nodejs#48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 8, 2023
@gabrielschulhof @ruyadorno
node-api: implement external strings
f93102c 
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 8, 2023
v18.18.0 proposal #49220
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@gabrielschulhof @ruyadorno
node-api: implement external strings
feb6a54 
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
@vmoroz vmoroz mentioned this pull request Nov 6, 2023
node-api: refactor napi_set_property function for improved performance #50282
Closed
@laverdet laverdet mentioned this pull request Nov 10, 2026
napi external string api completely broken in v26 #67370
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daeyeon daeyeon daeyeon approved these changes
@legendecas legendecas Awaiting requested review from legendecas
@mhdawson mhdawson Awaiting requested review from mhdawson

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose v8::String::NewExternalOneByte and v8::String::NewExternalTwoByte to Node-API

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@gabrielschulhof @nodejs-github-bot @daeyeon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.