-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: improve addons.markdown copy #4320
Conversation
ping ... @nodejs/collaborators @rvagg |
bb9b8a9
to
7dfd86f
Compare
|
||
- V8 JavaScript, a C++ library. Used for interfacing with JavaScript: | ||
creating objects, calling functions, etc. Documented mostly in the | ||
At the moment, The method for implementing Addons is rather complicated, |
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.
s/The/the
@rvagg ... thank you for the review! Pushed an update with fixes... still need to figure out #4320 (comment) tho... will look at that next |
@rvagg ... ok, added some language on the deps headers. PTAL |
- V8: the C++ library Node.js currently uses to provide the | ||
JavaScript implementation. V8 provides the mechanisms for creating objects, | ||
calling functions, etc. V8's API is documented mostly in the | ||
`V8.h` header file (`deps/V8/include/V8.h` in the Node.js source |
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.
The filenames should have use lowercase v8
.
@ofrobots ... fixed! |
creating objects, calling functions, etc. Documented mostly in the | ||
`v8.h` header file (`deps/v8/include/v8.h` in the Node.js source | ||
Node.js Addons are dynamically-linked shared objects, written in C or C++, that | ||
can be loaded into Node.js using the `require()` statement, and used just as if |
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.
require
is not a statement
General improvements to the documentation in addons.markdown.
66f9dbe
to
ed570d5
Compare
|
||
- Node.js includes a number of other statically linked libraries including | ||
OpenSSL. These other libraries are located in the `deps/` directory in the | ||
Node.js source tree. Only the v8 and OpenSSL symbols are purposefully |
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.
In few other places it is referred as V8.
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.
sigh lol... I keep missing these. Hopefully this is the last one
@thefourtheye ... fixed! |
@jasnell I didn't try all the code examples. But except the comment about dynamically linked thingy, everything else LGTM. |
+1 ... I dropped the "statically" in that one paragraph. |
Actually I would like to understand that paragraph better. So, if you don't mind, let's wait for one more LGTM. If the Addons were to dynamically link to V8, then the V8 has to be compiled and installed as a separate library in the target machine, right? Only then the Addons can load them at runtime. Is that the case here? |
The node binary exports the public symbols from libv8.a. Add-ons themselves don't load libv8, their references to V8 API functions are resolved by the dynamic linker at run-time to the ones from the node binary. |
Given the couple of days that have passed and no further comments, I'm going to go ahead and land this. |
General improvements to the documentation in addons.markdown. PR-URL: #4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Landed in d5863bc |
I'll handle porting these. |
General improvements to the documentation in addons.markdown. PR-URL: nodejs#4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@thealphanerd ... will be porting this to LTS early next week |
General improvements to the documentation in addons.markdown. PR-URL: nodejs#4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
General improvements to the documentation in addons.markdown. PR-URL: #4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
General improvements to the documentation in addons.markdown. PR-URL: #4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
General improvements to the documentation in addons.markdown. PR-URL: #4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
General improvements to the documentation in addons.markdown. PR-URL: nodejs#4320 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
General improvements to the documentation in addons.markdown.
/cc @nodejs/documentation