Repository navigation
vm.compileFunction should return an object #23923
Description
At #23837 (comment), @jdalton proposed that instead of returning a function with certain special properties, vm.compileFunction should instead return an object with the said properties and the function stored as the value property on the object.
This approach was infact discussed during the design phase of compileFunction, and I agree that either approach has it's own merits, but I decided to go with the status quo because it was consistent with similar functions in the vm module and consistency is really important IMO.
I'm opening this issue as suggested by @jdalton to open a discussion regarding what the best way to return information from these functions will be, moving forward.
I have three options in mind:
- Continue with the status quo (consistent).
- Make
vm.compileFunctionreturn an Object (inconsistent). - Make all similar functions return Objects (consistent).
Feel free to back one of the above or suggest something else entirely. The updates (if any) will have to be semver-major, but thankfully vm is probably one of the least used modules externally (vs internally).
Activity
ryzokuken commentedon Oct 27, 2018
/cc @nodejs/vm
jdalton commentedon Oct 28, 2018
Background: Before the properties like
cachedDataRejectedwere added to thescriptobject instance. BecausecompileFunctionreturns the actual function placing extra data likecachedDataRejectedends up on the actual function. Something like ascriptinstance would be handy for tracking this kind of data.Trott commentedon Nov 23, 2018
Should we try to ping a bit wider to try to get some opinions on this? (Alternatively, you can take the lack of responses as an indication that you should open a pull request to do whichever one you think makes the most sense. I don't know how much work that is, and how disappointing it would be to then have it rejected. On the other hand, responding to concrete pull requests is probably easier then forming opinions in the abstract?)
ryzokuken commentedon Nov 23, 2018
@Trott I think I could make a PR soon-ish.
github-actions commentedon Mar 8, 2022
There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.
github-actions commentedon Apr 8, 2022
There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.
For more information on how the project manages feature requests, please consult the feature request management document.
bnoordhuis commentedon Apr 27, 2022
@ryzokuken I see you've reopened this but IMO after ~4 years the ship has long sailed on making backwards incompatible changes.
ryzokuken commentedon Apr 27, 2022
@bnoordhuis that's understandable. Apologies for everyone for completely dropping the ball on this :/
That said, since this issue didn't get a whole lot of discussion, I suppose it's safe to assume that there aren't many concerns against the status quo.