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

external objects are not really JS objects #16

Closed
drsm opened this issue May 22, 2018 · 15 comments
Closed

external objects are not really JS objects #16

drsm opened this issue May 22, 2018 · 15 comments
Assignees
Labels
Milestone

Comments

@drsm
Copy link
Contributor

drsm commented May 22, 2018

$ ./njs
interactive njscript 0.2.1

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> typeof console
undefined
>> typeof console.log
undefined

Currently objects created externally (request from nginx module, for example) are implemented in a completely different way from native JS objects (NJS_EXTERNAL value type). It means that in every JS function a special branch is needed to support externals.

Instead external objects should be implemented using generic primitives used for native JS objects.
As a result external objects will be indistinguishable from native objects and will be supported by all JS functions.

@drsm drsm changed the title internal objects is not really an objecs internal objects is not really an objects May 22, 2018
@xeioex xeioex changed the title internal objects is not really an objects external objects are not really JS objects May 23, 2018
@xeioex xeioex added the bug label May 23, 2018
@drsm
Copy link
Contributor Author

drsm commented May 23, 2018

Hi @xeioex!
I'm mostly interested in behavior of its member functions, and been faced this issue while trying to implement something like this:

var logger;
function helper(args) {
  logger('some message');
}
function handler(req, res) {
  logger = req.error.bind(req);
  ...
}

Is it no-go, and i should use a closure here?

@xeioex xeioex added enhancement and removed bug labels May 23, 2018
@xeioex
Copy link
Contributor

xeioex commented May 23, 2018

Is it no-go, and i should use a closure here?

Yes, currently it is not possible. Using closures can be a way.

@xeioex
Copy link
Contributor

xeioex commented Jul 2, 2018

Partly fixed within #20.

@alveko
Copy link

alveko commented Dec 26, 2019

Do I understand it correctly that this issue makes things like JSON.stringify() not work for the objects constructed in externally in C-code (like the request object, for example)?

The issue is currently labeled as "in progress". Is it in progress? What's the reasonable timeline for the fix here? Is it weeks, months? Thanks!

@drsm
Copy link
Contributor Author

drsm commented Dec 26, 2019

@alveko
Hi!
Take a look at #192.

@alveko
Copy link

alveko commented Dec 26, 2019

@drsm thanks! Ok, so JSON.stringify() is supported now.

The second part of the question remains. Do you guys plan to make the external object indistinguishable from the native JS objects as per this issue? Is the in-progress state accurate, and if yes, when do you think it may be delivered? I'm not asking you to commit to any dates, of course, but any kind of heads up on the timeline is much appreciated.

@xeioex
Copy link
Contributor

xeioex commented Dec 27, 2019

@alveko

Do you guys plan to make the external object indistinguishable from the native JS objects as per this issue?

Yes, we do.

Is the in-progress state accurate, and if yes, when do you think it may be delivered?

currently it is on pause. I plan to do this in 3-month timeframe.

@xeioex
Copy link
Contributor

xeioex commented Mar 17, 2020

@drsm @hongzhidao

Take a look at the following patch (beta): https://gist.github.com/112de4cc2de34a9dbdccbc6bbbcd23c3

Now external definitions are converted to native JS object primitives. External queries like in r.headersOut are implemented using mechanism similar to exotic slots from the spec.

Definition example:

  {                                                                                                                          
          .flags = NJS_EXTERN_OBJECT,                                                                                            
          .name.string = njs_str("headersOut"),    // may also be a symbol                                                                              
          .writable = 1,                                                                                                         
          .configurable = 1,                                                                                                     
          .enumerable = 1,                                                                                                       
          .u.object = {                               
                 // may implement [[Get]] (and [[Set]], [[Delete]] if writable or configurable is true)                                                       
              .prop_handler = ngx_http_js_ext_header_out,  // njs_prop_handler_t                                                                      
              .keys = ngx_http_js_ext_keys_header_out,                                                                           
          }                                                                                                                      
      },                                                                                                                         
          

Result

>> console.toString()
'[object Console]'
>> console[0]='a'; console[1]='b'; console.length =2; console.__proto__ = Array.prototype; console.join('|')
'a|b'
>> Object.getOwnPropertyNames(console)
[
 'log',
 '0',
 '1',
 'length',
 'dump',
 'time',
 'timeEnd'
]
>> JSON.stringify(console)
'{"0":"a","1":"b","length":2}'
>> console
Console {
 log: [Function: native],
 0: 'a',
 1: 'b',
 length: 2,
 dump: [Function: native],
 time: [Function: native],
 timeEnd: [Function: native]
}

@hongzhidao
Copy link
Contributor

@xeioex

Now external definitions are converted to native JS object primitives. External queries like in r.headersOut are implemented using mechanism similar to exotic slots from the spec.

I'm wondering when you plan to commit? I need to deep into code again, it changes a lot.

  1. Now, what is the passing rate of test cases?

  2. After this, what do you plan to continue in 0.4.0?

@xeioex
Copy link
Contributor

xeioex commented Mar 18, 2020

@hongzhidao

I'm wondering when you plan to commit?

By the end of this week.

Now, what is the passing rate of test cases?

 zero. Because our external objects are not covered by test262.

After this, what do you plan to continue in 0.4.0?

Short term plan includes:

  1. setting multiple outgoing headers with the same name (for example 'Set-Cookie') #266 (adding API to change/modify Cookie headers).
  2. js_import directive. (To avoid js_include altogether.)

nginx.conf:


js_import lib1 from 'lib1.js'; 
js_import lib2 from 'lib2.js';

# main file will be implicitly generated as follows:
# import lib1 from  'lib1.js'; 
# import lib2 from  'lib2.js'; 

location /lib1 {
     js_content "lib1.handler";
}

location /lib2 {
     js_content "lib2.handler";
}

@hongzhidao
Copy link
Contributor

hongzhidao commented Mar 19, 2020

@xeioex
External objects become much better than before. And take a look at two places.

  1. adjust the order of the fields in njs_object_t. __proto__, hash, shared_hash, and slots.
    And it seems that __proto__ does not need comment.

  2. What about renaming njs_exotic_slots_t as njs_exotic_slot_t.
    As I understand, slot is similar to prop/property. In spec, there are internal and exotic objects. They have corresponding properties and methods, they are internal and exotic.
    Now have njs_object_prop_t. It's good to introduce njs_exotic_slots_t.
    slots (array) is good a trick for storing nested external objects.
    But it seems every object has one slot, is it right?
    If yes, I prefer to use slot to slots.

typedef struct {
    njs_lvlhsh_t        hash;
    ...
    njs_exotic_keys_t   keys;
} njs_exotic_slot_t;


struct njs_object_s {
    njs_object_t                      *__proto__;

    /* A private hash of njs_object_prop_t. */
    njs_lvlhsh_t                      hash;

    /* A shared hash of njs_object_prop_t. */
    njs_lvlhsh_t                      shared_hash;

    /* TODO */
    njs_exotic_slot_t                 *slot;

    /* The type is used in constructor prototypes. */
    njs_value_type_t                  type:8;
    ...
};

Others look good.
Do you plan to continue refactoring this part?
I mean object, property, exotic. Is it totally finished?

 *
 *   TODO:
 *     Object.defineProperty([1,2], '1', {configurable:false})
 */

njs_int_t
njs_property_query(njs_vm_t *vm, njs_property_query_t *pq, njs_value_t *value,
    njs_value_t *key)
{

BTW, how do you get njs_exotic_slots_t? it's good.
Do you make it clear before start coding? Or it appears while coding and take inspiration?
It's hard for me to make things clear before start to do it.
I often use the second way, but it always needs to change code many times.

@hongzhidao
Copy link
Contributor

@xeioex

js_import directive. (To avoid js_include altogether.)

Does it mean js_include will be deprecated?

It's common that we need to do the same things on many servers. Such as authentication.
How to use it?

Is anyone on it? If not, I'd like to try this.

@xeioex
Copy link
Contributor

xeioex commented Mar 19, 2020

@hongzhidao

  1. And it seems that proto does not need comment.

agree.

  1. But it seems every object has one slot, is it right?
    If yes, I prefer to use slot to slots.

https://tc39.es/ecma262/#sec-object-internal-methods-and-internal-slots

In the spec, the slots correspond to [[...]] methods. The exotic slots are used to redefine the standard behavior or different built-methods like ([[Get]], [[Set]], [[Delete]], [[Keys]], ..). I believe njs_exotic_slots_t (plural) is better because the are many methods that can be redefined (some will be added later).

In my definition [[Get]], [[Set]], [[Delete]] are merged to a single prop_handler. Flags specify in which context prop_handler can be invoked.

typedef struct {                                                                                                                 
//...                                                                              
    njs_prop_handler_t  prop_handler;                                                                                            
    unsigned            writable:1;                                                                                              
    unsigned            configurable:1;                                                                                          
    unsigned            enumerable:1;                                                                                            
                                                                                                                                 
    njs_exotic_keys_t   keys;   
// will be more later for other types                                                                                                 
} njs_exotic_slots_t; 
/* ..
 *   TODO:
 *     Object.defineProperty([1,2], '1', {configurable:false})
 */

njs_int_t
njs_property_query(njs_vm_t *vm, njs_property_query_t *pq, njs_value_t *value,
    njs_value_t *key)
{

Good catch, the issue already fixed in 58ebb38. The comment will be removed.

Do you make it clear before start coding? Or it appears while coding and take inspiration?
It's hard for me to make things clear before start to do it.

I had some general idea and refined it in the process of development.

@xeioex
Copy link
Contributor

xeioex commented Mar 19, 2020

@hongzhidao

Does it mean js_include will be deprecated?

most probably yes, but far into the future.

It's common that we need to do the same things on many servers. Such as authentication.
How to use it?

yes. that is why splitting your logic into different modules helps. Now, you have to merge all of your code, probably for different, unrelated locations into one js_include file.

Is anyone on it? If not, I'd like to try this.

not yet, I plan to do this after this issue.

@xeioex
Copy link
Contributor

xeioex commented Apr 23, 2020

@alveko

njs 0.4.0 is released: http://nginx.org/en/docs/njs/changes.html#njs0.4.0

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

No branches or pull requests

4 participants