FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: improve addons.markdown copy by jasnell · Pull Request #4320 · nodejs/node · GitHub

/ node Public

doc: improve addons.markdown copy - #4320

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:doc-addon-improvements
ConversationCommits (2)ChecksFiles changed
Closed

doc: improve addons.markdown copy#4320
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:doc-addon-improvements

Conversation

jasnell commented Dec 17, 2015

Copy link
Copy Markdown
Member

General improvements to the documentation in addons.markdown.

/cc @nodejs/documentation

jasnell added doc Issues and PRs related to the documentations. lts-watch-v4.x labels Dec 17, 2015

jasnell commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

ping ... @nodejs/collaborators @rvagg

jasnell force-pushed the doc-addon-improvements branch from bb9b8a9 to 7dfd86f Compare December 18, 2015 01:28
rvagg reviewed Dec 18, 2015
View reviewed changes
Comment thread doc/api/addons.markdown Outdated

rvagg Dec 18, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

s/The/the

jasnell commented Dec 18, 2015

Copy link
Copy Markdown
Member Author

@rvagg ... thank you for the review! Pushed an update with fixes... still need to figure out #4320 (comment) tho... will look at that next

jasnell commented Dec 18, 2015

Copy link
Copy Markdown
Member Author

@rvagg ... ok, added some language on the deps headers. PTAL

ofrobots reviewed Dec 18, 2015
View reviewed changes
Comment thread doc/api/addons.markdown Outdated

ofrobots Dec 18, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The filenames should have use lowercase v8.

jasnell commented Dec 18, 2015

Copy link
Copy Markdown
Member Author

@ofrobots ... fixed!

thefourtheye reviewed Dec 19, 2015
View reviewed changes
Comment thread doc/api/addons.markdown Outdated

thefourtheye Dec 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

require is not a statement

doc: improve addons.markdown copy
ed570d5 
General improvements to the documentation in addons.markdown.
jasnell force-pushed the doc-addon-improvements branch from 66f9dbe to ed570d5 Compare December 21, 2015 19:04
thefourtheye reviewed Dec 22, 2015
View reviewed changes
Comment thread doc/api/addons.markdown Outdated

thefourtheye Dec 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In few other places it is referred as V8.

jasnell Dec 22, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

sigh lol... I keep missing these. Hopefully this is the last one

jasnell commented Dec 22, 2015

Copy link
Copy Markdown
Member Author

@thefourtheye ... fixed!

thefourtheye commented Dec 22, 2015

Copy link
Copy Markdown
Contributor

@jasnell I didn't try all the code examples. But except the comment about dynamically linked thingy, everything else LGTM.

jasnell commented Dec 22, 2015

Copy link
Copy Markdown
Member Author

+1 ... I dropped the "statically" in that one paragraph.

thefourtheye commented Dec 22, 2015

Copy link
Copy Markdown
Contributor

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?

bnoordhuis commented Dec 22, 2015

Copy link
Copy Markdown
Member

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.

jasnell commented Dec 23, 2015

Copy link
Copy Markdown
Member Author

Given the couple of days that have passed and no further comments, I'm going to go ahead and land this.

jasnell added a commit that referenced this pull request Dec 23, 2015
doc: improve addons.markdown copy
d5863bc 
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>

jasnell commented Dec 23, 2015

Copy link
Copy Markdown
Member Author

Landed in d5863bc

jasnell closed this Dec 23, 2015

MylesBorins commented Dec 30, 2015

Copy link
Copy Markdown
Contributor

this one is merging with conflicts, and I believe there is a second commit @rvagg to fix some regressions this created. @jasnell can you take the lead on this one?

jasnell commented Dec 30, 2015

Copy link
Copy Markdown
Member Author

I'll handle porting these.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
doc: improve addons.markdown copy
57684d6 
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>
MylesBorins mentioned this pull request Jan 28, 2016
Closed

jasnell commented Jan 29, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd ... will be porting this to LTS early next week

rvagg mentioned this pull request Feb 16, 2016
Closed
jasnell added a commit to jasnell/node that referenced this pull request Feb 19, 2016
doc: improve addons.markdown copy
c716161 
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>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
doc: improve addons.markdown copy
f513118 
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>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
doc: improve addons.markdown copy
a22f688 
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>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
doc: improve addons.markdown copy
b18fd41 
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
doc: improve addons.markdown copy
41c1f71 
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>
snyk-bot mentioned this pull request Feb 12, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL