New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 5979 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
HW: ----
NextAction: ----
OS: ----
Priority: 2
Type: FeatureRequest

Blocking:
issue 1569



Sign in to add a comment

Module API: Instantiate could take a data arg, Module could have an internal field

Project Member Reported by adamk@chromium.org, Feb 17 2017

Issue description

From kozyatinskiy@ on  issue 1569  (and also mentioned separately on #v8 by bradleymeck):

Bogdan Padalko asked me to post this comment by email:

I currently work on php-v8 (https://github.com/pinepain/php-v8) to add experimental modules support. 

Is it possible to add data as an optional third parameter to Module::Instantiate which will be then passed as a fourth parameters to Module::ResolveCallback callback:

V8_WARN_UNUSED_RESULT bool Instantiate(Local<Context> context,
ResolveCallback callback,
Local<Value> data = Local<Value>());

and

typedef MaybeLocal<Module> (*ResolveCallback)(Local<Context> context,
Local<String> specifier,
Local<Module> referrer,
Local<Value> data);

I'd benefit from it a lot to pass PHP callback. I guess all embedders would benefit from this.

Also, it would be quite nice for embedding purposses to be able to set internal fields on Module, like on Context to associate embedders data.
 

Comment 1 Deleted

Comment 2 Deleted

Comment 3 Deleted

Thanks for posting it for me and taking care, for some reason I wasn't able to get in with 3rd party cookie disabled.

I found that this was removed some time ago - https://github.com/v8/v8/commit/9d2051fc2896bcb771cfb41b2864601c6c40e299, CR - https://codereview.chromium.org/2405313002. While the rationale is valid for d8, that removal trim delegating all of logic to embedder and makes embedding a rusty process.

Is it feasible to revert (partially?) that commit changes in part of Module API?

Comment 5 by adamk@chromium.org, Feb 17 2017

Something like reverting that change is indeed a way to fix this issue, but I want to make sure that it doesn't cause other problems before going down that path.

Note that the current API is not meant to be taken as final (as indicated by the comments that say "experimental, do not use" :).
Thank you for a quick reply. I don't take API as final, just trying to see what it might be good for and try to grasp brief idea how to embed it.

I have few question about this experimental es modules API which may be or not a bugs (If mail list is a better place for such questions I'll post it there):
 1. At this time, if I get it right, Module::GetIdentityHash() may not be unique, right? At least I got hashes collision (about 20-25k modules nested in depth, not a typical use case).
 2 For some reason imported object has no CreationContext() set, at least v8 bail out when I try to use it. Here I'm not 100% confident that this is exact problem, though, you definitely have better knowledge here:

outer: import * as test from "inner.js"; console.log(test);
inner: export const foo = "bar";

When I try to access test->CreationContext() with v8 5.8.172


#
# Fatal error in ../../src/objects.cc, line 3348
# Check failed: receiver->IsJSFunction().
#

==== C stack trace ===============================

    /opt/libv8-5.8/lib/libv8_libbase.so(+0xfb3e) [0x7f40bd456b3e]
    /opt/libv8-5.8/lib/libv8_libbase.so(V8_Fatal+0xdf) [0x7f40bd454a8f]
    /opt/libv8-5.8/lib/libv8.so(+0x62cfc5) [0x7f40b8466fc5]
    /opt/libv8-5.8/lib/libv8.so(v8::Object::CreationContext()+0xc) [0x7f40b7fd9e7c]
    /home/vagrant/Development/php-v8-wrapper/php-v8/modules/v8.so(php_v8_object_get_self_ptr(v8::Isolate*, v8::Local<v8::Object>)+0x3d) [0x7f40b8bd3c9d]
    /home/vagrant/Development/php-v8-wrapper/php-v8/modules/v8.so(php_v8_get_or_create_value(_zval_struct*, v8::Local<v8::Value>, v8::Isolate*)+0x24) [0x7f40b8bc6cd4]
    /home/vagrant/Development/php-v8-wrapper/php-v8/modules/v8.so(+0x5aa7c) [0x7f40b8bdfa7c]
    /usr/bin/php7.1(+0x30e8ac) [0x562c7e8ab8ac]
    /usr/bin/php7.1(execute_ex+0x2b) [0x562c7e852eeb]
    /usr/bin/php7.1(zend_call_function+0x7df) [0x562c7e7f9f4f]
    /home/vagrant/Development/php-v8-wrapper/php-v8/modules/v8.so(php_v8_callback_call_from_bucket_with_zargs(unsigned long, v8::Local<v8::Value>, _zval_struct*, _zval_struct*)+0xec) [0x7f40b8baf86c]
    /home/vagrant/Development/php-v8-wrapper/php-v8/modules/v8.so(php_v8_callback_function(v8::FunctionCallbackInfo<v8::Value> const&)+0x82) [0x7f40b8baf992]
    /opt/libv8-5.8/lib/libv8.so(+0x1828c9) [0x7f40b7fbc8c9]
    /opt/libv8-5.8/lib/libv8.so(+0x20d886) [0x7f40b8047886]
    /opt/libv8-5.8/lib/libv8.so(+0x20cdef) [0x7f40b8046def]
    [0x27df512043a7]
Illegal instruction (core dumped)

Labels: Priority-2

Comment 8 by adamk@chromium.org, Aug 2 2017

Status: Fixed (was: Assigned)
I don't think we're likely to change this fact about the API anytime soon.

Sign in to add a comment