-
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
Deprecating _third_party_main #24017
Comments
I was talking about this with @rubys at JSI – They were looking into splitting the bootstrap function into two parts as a better way for addressing the embedder use case here (the parts are a) basically everything that happens before If embedders are given the option of only executing bootstrap code up to the point where we’d currently call |
@addaleax I agree. Giving people a better alternative then deprecating |
See #23265 |
Since it's undocumented and is a compile-time/build option, I don't think the doc-deprecation and runtime-deprecations are applicable. I think we can go right to a semver-major removal, if that's what we wish to do. I guess that should wait for the alternative approach proposed by @rubys and @addaleax? Is that something that is in the "oh yeah, we're totally implementing it" phase? Or more of the "we kicked this idea around and kinda like it, but that's as far as we got" phase? |
This issue saw its anniversary last Friday. Should it remain open? |
I would keep it open for a bit, I’ve started doing some more work towards a proper embedder API and maybe something will come out of that in terms of a better solution than this for embedders. |
Ping @addaleax ... where are we at in terms of moving forward on this? |
@jasnell Given that an alternative exists now, I think we could move forward here, but it’s a bit tricky:
Ultimately, I think the closest thing we have to this is the existing deprecation of I’ll open a PR for removal and we’ll see how that goes. |
That works, I think. |
Since 7dead84, there is a more official alternative that is tested and comes with a proper API, and since a6c57cc, the `LoadEnvironment(env)` overload is deprecated, which is the closest thing we can achieve to deprecating `_third_party_main` support. Thus, we can now consider us able to remove `_third_party_main` support. Fixes: nodejs#24017 Refs: nodejs#30467 Refs: nodejs#32858
Removal PR → #33971 |
@addaleax what's the alternative to |
@addaleax - thanks for the quick reply! I was just to about to update the comment with the link - as the author of js2bin I'm trying to figure out how to integrate with the new API without having to deal with patching the |
@ledbit Yeah, I admit that that’s a bit tricky, I ran into this with boxednode as well and I’m not super happy about having to patch the gyp files (although there I do that mostly for addon support). Tbh, I think the real solution here would be for Node.js to publish usable static library files as part of its release process alongside the executable binaries for the different platforms. |
@addaleax I was taking a look at the node init sequence and at first pass it seems like there are quite a few things which would need to be duplicated and kept in sync - e.g. the use of
There are also quite a few other signal handling / debugging tracing - e.g. the lack of calling boxednode looks very interesting, I'll research that more in the AM |
+100 for a static library @addaleax 😂 |
@addaleax and @calebboyd maybe I'm missing something obvious here, but how exactly would a static library help? Also, the new embedding API seems quite a bit more cumbersome and prone to maintenance overhead as Node's startup sequence evolves - e.g there's quite a bit of code in boxednode that is lifted from node.cc (albeit with a note to expose an API on the node side - pointing to more work to be done on both node and boxednode side) My $0.02: the |
I think @ledbit's issue comes down to "how can we tell Node.js to embed a JS starting point during the building process without having to patch node.gyp". I wonder though why haven't we ever tried implementing this with a configure option? Like |
@joyeecheung - exactly! With the |
I stumbled upon some very old code in the loader with the following code comment:
So instead of the regular loader it is going to load the new file. However, that is only the case if that file is also added to node.gyp. This "feature" has never been documented and seems to be used very rarely when searching for it with google. I am not really sure if the best way to deal with this is to document this or to deprecate it but keeping it as is does not seem to be ideal. Is this something we really want to support officially? This functionality seems rather odd to me.
Ping @nodejs/tsc
The text was updated successfully, but these errors were encountered: