Skip to content
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

CompileFunctionInContext is deprecated #214

Closed
targos opened this issue Oct 28, 2021 · 10 comments
Closed

CompileFunctionInContext is deprecated #214

targos opened this issue Oct 28, 2021 · 10 comments

Comments

@targos
Copy link
Member

targos commented Oct 28, 2021

[850/3712] CXX obj/deps/icu-small/source/common/icutools.servlkf.o
../../src/node_contextify.cc:1178:51: warning: 'CompileFunctionInContext' is deprecated: Use CompileFunction [-Wdeprecated-declarations]
  MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunctionInContext(
                                                  ^
../../deps/v8/include/v8-script.h:691:3: note: 'CompileFunctionInContext' has been explicitly marked deprecated here
[850/3712] CXX obj/deps/icu-small/source/common/icutools.stringpiece.o  V8_DEPRECATE_SOON("Use CompileFunction")
  ^
../../deps/v8/include/v8config.h:462:39: note: expanded from macro 'V8_DEPRECATE_SOON'
# define V8_DEPRECATE_SOON(message) [[deprecated(message)]]
                                      ^
1 warning generated.
[1133/2485] CXX obj/src/libnode.node_native_module.o
../../src/node_native_module.cc:278:23: warning: 'CompileFunctionInContext' is deprecated: Use CompileFunction [-Wdeprecated-declarations]
      ScriptCompiler::CompileFunctionInContext(context,
                      ^
../../deps/v8/include/v8-script.h:691:3: note: 'CompileFunctionInContext' has been explicitly marked deprecated here
  V8_DEPRECATE_SOON("Use CompileFunction")
  ^

See https://source.chromium.org/chromium/_/chromium/v8/v8.git/+/78387ca75dc95e39118805fa72e52aea12a37a66

It's not trivial to migrate, because script_or_module_out isn't exposed anymore but we use it.

@devsnek
Copy link
Member

devsnek commented Oct 28, 2021

Looking at the diff it seems like maybe we can do CompileFunction()->GetUnboundScript()->BindToCurrentContext()? nvm i see the issue. this is quite annoying :(

@targos
Copy link
Member Author

targos commented Oct 31, 2021

@nodejs/v8 do you have any suggestions on how we could migrate to the new API?

@hashseed
Copy link
Member

hashseed commented Nov 2, 2021

@camillobruni

@camillobruni
Copy link

This is the first part of a multistage fix to get around the implementation issues with host-defined options :(.

  • CompileFunctionInContext works for now for now (minus the deprecation):
    • It creates temporary ScriptOrModule objects (slow, but backwards compatible)
    • It references the temporary ScriptOrModule objects from the Script if the v8_scriptormodule_legacy_lifetime is set to be backwards compatible with node

Pending Work:

  • The main fix is ready for V8 but pending on nodejs fixes for modules
  • Host-defined options will be allowed to be an arbitrary embedder-defined object (increased risk of leaks, but greatly improved ergonomics). This should fix node's current hack to keep module-metadata alive via ScriptOrModule lifetimes.

@camillobruni
Copy link

For the time being I could un-deprecate CompileFunctionInContext (since I've updated V8 + blink already). WDYT?

@devsnek
Copy link
Member

devsnek commented Nov 2, 2021

Host-defined options will be allowed to be an arbitrary embedder-defined object 

I think as long as this is out before CompileFunctionInContext is removed we can keep using it deprecated for now.

It sounds like we should enable v8_scriptormodule_legacy_lifetime though.

@camillobruni
Copy link

I plan to only remove CompileFunctionInContext once the complete API migration is complete.

@targos
Copy link
Member Author

targos commented Nov 5, 2021

@camillobruni I think this is the source of the errors in #213.

I'm wondering why your Node.js integration tests don't catch them.

@camillobruni
Copy link

So far we don't have a bot that fails with deprecation warnings (based on your comment we're likely adding a compile-only bot with deprecation warnings on).

In general we don't really have the capacity to fix node full time, but in this case communication might have not been ideal. We try to limit API churn for as much as possible, but there is quite some work ahead to make modules work properly (see https://crbug.com/1244145), so I apologise in advance for the noise this will generate.

@targos
Copy link
Member Author

targos commented Nov 5, 2021

Sorry, I missed @devsnek's comment. The solution to #213 was to enable v8_scriptormodule_legacy_lifetime.

targos added a commit to targos/node that referenced this issue Nov 22, 2021
targos added a commit to targos/node that referenced this issue Jan 18, 2022
targos added a commit to targos/node that referenced this issue Jan 18, 2022
targos added a commit to nodejs/node that referenced this issue Jan 20, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: #40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to targos/node that referenced this issue Jan 20, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to nodejs/node that referenced this issue Jan 20, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: #40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to targos/node that referenced this issue Jan 29, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to targos/node that referenced this issue Feb 1, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to nodejs/node that referenced this issue Feb 2, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: #40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to nodejs/node that referenced this issue Feb 2, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: #40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to targos/node that referenced this issue Feb 2, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
VoltrexKeyva pushed a commit to VoltrexKeyva/node that referenced this issue Feb 3, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to targos/node that referenced this issue Feb 23, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: nodejs#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to nodejs/node that referenced this issue Feb 24, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: #40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Feb 24, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Feb 25, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Feb 25, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Feb 26, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to nodejs/node that referenced this issue Jul 30, 2022
We are not ready to migrate yet.

Refs: nodejs/node-v8#214

PR-URL: #40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Jul 30, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Jul 31, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 1, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 2, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 3, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 4, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 5, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 6, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 7, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 8, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 9, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 10, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 11, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 12, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 13, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 14, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 15, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this issue Aug 16, 2022
V8 APIs like HostImportModuleDynamicallyCallback and
ScriptCompiler::CompileFunction is moving away from ScriptOrModule.

Replaces ScriptCompiler::CompileFunctionInContext with
ScriptCompiler::CompileFunction to remove the usages on the optional
out param ScriptOrModule.

PR-URL: #44198
Fixes: nodejs/node-v8#214
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 16, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Aug 17, 2022
We are not ready to migrate yet.

Refs: #214

PR-URL: nodejs/node#40907
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
ruyadorno pushed a commit to nodejs/node that referenced this issue Aug 23, 2022
V8 APIs like HostImportModuleDynamicallyCallback and
ScriptCompiler::CompileFunction is moving away from ScriptOrModule.

Replaces ScriptCompiler::CompileFunctionInContext with
ScriptCompiler::CompileFunction to remove the usages on the optional
out param ScriptOrModule.

PR-URL: #44198
Fixes: nodejs/node-v8#214
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
V8 APIs like HostImportModuleDynamicallyCallback and
ScriptCompiler::CompileFunction is moving away from ScriptOrModule.

Replaces ScriptCompiler::CompileFunctionInContext with
ScriptCompiler::CompileFunction to remove the usages on the optional
out param ScriptOrModule.

PR-URL: nodejs#44198
Fixes: nodejs/node-v8#214
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
orgads added a commit to orgads/isolated-vm that referenced this issue Jan 16, 2024
CompileFunctionInContext was removed.

See nodejs/node-v8#214.

Fixes laverdet#436.
orgads added a commit to orgads/isolated-vm that referenced this issue Jan 16, 2024
CompileFunctionInContext was removed.

See nodejs/node-v8#214.

Fixes laverdet#436.
laverdet pushed a commit to laverdet/isolated-vm that referenced this issue Jan 16, 2024
CompileFunctionInContext was removed.

See nodejs/node-v8#214.

Fixes #436.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants