Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
doc: how to expose primordials when using --expose-internals#38026
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ljharb commented Apr 1, 2021
Seems fine; it might be nice to have the primordials exposed as a core module always, too. |
bmeck commented Apr 1, 2021
@ljharb |
ljharb commented Apr 1, 2021
Ah, I’d say that’s a requirement. In that case tho, is it even good to expose them with this flag? |
bmeck commented Apr 1, 2021
@ljharb I'd say the benefit of developing internal packages in different repos likely outweighs the problems. Using |
jasnell left a comment
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.
--expose-internals is problematic on it's own but there's really no great alternative, and it's super useful for testing so I'm +1 on this.
bmeck commented Apr 1, 2021
@jasnell I do think moving |
jasnell commented Apr 1, 2021
Agreed |
nodejs-github-bot commented Apr 1, 2021
aduh95 commented Apr 1, 2021
FWIW you can achieve this already since 983e922: Doesn't this cover your use-case already? |
bmeck commented Apr 2, 2021
@aduh95 maybe we could just add this to testing docs then? |
aduh95 commented Apr 2, 2021
Yes definitely, the fact that you don't know about this shows that it's not documented well enough. Do you want me to open a new PR or would you like to re-purpose this one? |
bmeck commented Apr 2, 2021
I can repurpose this one in a bit |
efb5dd8 to 2facf36Compare| ``` | ||
| In specific scenarios it may be useful to get a hold of `primordials` or | ||
| `internalBinding()`. You can do so using |
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.
| `internalBinding()`. You can do so using | |
| `internalBinding()`. You can do so using: |
Trott left a comment
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.
LGTM with or without my trivial suggestion.
This comment has been minimized.
This comment has been minimized.
bmeck commented Apr 5, 2021
updated title |
nodejs-github-bot commented Apr 5, 2021
PR-URL: nodejs#38026 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
2facf36 to f52c921CompareTrott commented Apr 6, 2021
Landed in f52c921. @bmeck If it saves you a little time in the future, doc-only changes don't need to be run on Jenkins. You can rely on the GitHub Actions results for those. (The one possible exception would be the docs for addons. The C++ code in the examples are extracted and compiled to make sure they are all valid. I'm not sure if that happens on GitHub Actions or not. But everything else...doc-only changes don't need Jenkins.) |
bmeck commented Apr 6, 2021
@Trott I tried to re-read collaborator guide for that but guess I missed it. |
Trott commented Apr 6, 2021
Understandable considering how overwhelming the guide is. You are now inspiring me to open a pull request to remove non-essential content. |
PR-URL: #38026 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #38026 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #38026 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
This exposes
primordialsas a global property when using--expose-internals.This allows efforts like #21128 to follow general core robustness using
primordialswhile potentially being developed outside of the core repo itself. Without such a repo usingprimordialsit would not be easy to vendor in while maintaining such rigor without various duplications.The
--expose-internalsflag is not currently documented in the main docs and I didn't add a note about this because of that.