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

doc: add WebAssembly to globals #23339

Closed
wants to merge 5 commits into from
Closed

doc: add WebAssembly to globals #23339

wants to merge 5 commits into from

Conversation

styfle
Copy link
Member

@styfle styfle commented Oct 8, 2018

Fixes #23334

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 8, 2018
doc/api/globals.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 8, 2018

Possibly unrelated to this PR, but it seems like there's some room for cleanup? Class: WebAssembly seems right to me, but we don't prepend TextDecoder with Class:, so... somebody probably should go through the doc and figure out a consistent formatting for classes? /cc @nodejs/documentation

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Left a nit or two, but this is an improvement as is, so I'm happy for this to land with or without changes for the nits.

@Trott Trott added the wasm Issues and PRs related to WebAssembly. label Oct 8, 2018
@styfle
Copy link
Member Author

styfle commented Oct 8, 2018

@Trott Thanks for the quick review!

Do you know why I'm getting this lint error? "blank line expected after title" line-after-title

@Trott
Copy link
Member

Trott commented Oct 8, 2018

The Travis linter error about the commit message needing a blank line after the title can be ignored. It's a known issue with core-validate-commit that will be fixed when we publish a new release of core-validate-commit.

Ref: nodejs/core-validate-commit#29

I'll ping the people with publishing ability.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 8, 2018

Can we really name it as Class considering this note?

Unlike most other global objects, WebAssembly is not a constructor (it is not a function object). You can compare it to Math, which is also a namespace object for mathematical constants and functions, or to Intl which is the namespace object for internationalization constructors and other language sensitive functions.

@styfle
Copy link
Member Author

styfle commented Oct 9, 2018

@vsemozhetbyt Good point, I removed the "Class" prefix. Eventually, it could probably be removed from Buffer too as Rich mentioned above.

doc/api/globals.md Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 9, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1180/

@styfle
Copy link
Member Author

styfle commented Oct 9, 2018

Thanks all! Anything else I need to do (should I fix the commit message)?

@Trott
Copy link
Member

Trott commented Oct 10, 2018

Thanks all! Anything else I need to do (should I fix the commit message)?

Nothing else to do. This needs to stay open another 19 hours or else be fast-tracked, but I don't think we need it to be fast-tracked unless we're trying to get it into the 10.12.0 release? Which actually might not be a bad idea.

Collaborators, 👍 here to fast-track.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 10, 2018
@Trott
Copy link
Member

Trott commented Oct 10, 2018

Landed in 487020e

@Trott Trott closed this Oct 10, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 10, 2018
PR-URL: nodejs#23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@styfle styfle deleted the patch-2 branch October 11, 2018 12:43
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
PR-URL: #23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. wasm Issues and PRs related to WebAssembly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebAssembly global is not mentioned in documentation
8 participants