-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: how to expose primordials when using --expose-internals #38026
Conversation
Seems fine; it might be nice to have the primordials exposed as a core module always, too. |
@ljharb |
Ah, I’d say that’s a requirement. In that case tho, is it even good to expose them with this flag? |
@ljharb I'd say the benefit of developing internal packages in different repos likely outweighs the problems. 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.
--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.
@jasnell I do think moving |
Agreed |
FWIW you can achieve this already since 983e922:
Doesn't this cover your use-case already? |
@aduh95 maybe we could just add this to testing docs then? |
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? |
I can repurpose this one in a bit |
efb5dd8
to
2facf36
Compare
@@ -286,6 +286,15 @@ const assert = require('assert'); | |||
const freelist = require('internal/freelist'); | |||
``` | |||
|
|||
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: |
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.
updated title |
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
f52c921
Compare
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.) |
@Trott I tried to re-read collaborator guide for that but guess I missed it. |
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
primordials
as a global property when using--expose-internals
.This allows efforts like #21128 to follow general core robustness using
primordials
while potentially being developed outside of the core repo itself. Without such a repo usingprimordials
it would not be easy to vendor in while maintaining such rigor without various duplications.The
--expose-internals
flag is not currently documented in the main docs and I didn't add a note about this because of that.