Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
src: add support for externally shared js builtins #44376
src: add support for externally shared js builtins #44376
Changes from 1 commit
b7d4bcd
5428aa2
5dd4c99
6f6d80e
a614c7b
eb708c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I assume this means that distros can now provide different versions of these dependencies. Are we okay with that?
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.
I see this as very similar to supporting external shared libraries and it is motivated by a similar use case. Since it does not affect what we ship I think it's up to the distros that use it to keep the versions the same or compatible in what they ship just like they do for shared libraries.
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.
I think we need to carefully weigh this. I see the motivation but not quite convinced yet.
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.
@jasnell can you outline your concerns in a bit more detail? We have 2 distro's asking for the functionality in order to avoid having to hack in their own solutions?
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.
This probably needs some sort of
#ifdef
guard for--without-intl
?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.
@addaleax I was wondering about that. If we don't have intl, do you know how typically convert to utf16 if intl is not enabled ?
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.
#37954 (comment) has some prior discussion on that (tl;dr: we don’t, we can use V8 in cases in which an Isolate is available but not here)
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.
Thanks for the pointer to the discussion, I can see that is trying to the code where I'd grabbed using icu for the conversion from :)
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.
I ended up making it so that the --without-intl reports an error if you try to configure it along with an externally shared js builtin and then guarding the code so that if --without-intl is used without the new options it is excluded.
My take is that it is acceptable for the new options to not be availble when --without-intl. My reasoning is both that people using --without-intl is a small set of people at this point in time and that those that do are unlikely to be interested in externalizing builtins and that rolling our own icu convertion for this unlikely case is not worth the work/additional code in Node.js. If that's wrong or we add native conversion helpers at some later time we can always do the work to remove that restriction.
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.
I also did confirm as Anna mentioned that an isolate is not available in the method where we do the coversions so using V8 is not possible in this case.