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

JS API: initial implementation #147

Closed
wants to merge 2 commits into from
Closed

Conversation

joyeecheung
Copy link
Member

Fixes: #14

Based on the previous work done by @rnchamberlain in #14 (comment)

Overview of the API is documented as JSDoc annotations in JSAPI.md.

Still needs a bit of clean up, but any feedback on the API design would be very welcome!

joyeecheung

This comment was marked as off-topic.

bnoordhuis

This comment was marked as off-topic.

@joyeecheung joyeecheung changed the title [WIP] JS API: initial implementation JS API: initial implementation Nov 29, 2017
@joyeecheung
Copy link
Member Author

(Replied here in case it's buried in folded comments..)

fs.statSync('/usr/lib/llvm-4.0/lib/liblldb-4.0.so') throws an ENOENT, on my ubuntu box it's

lrwxrwxrwx 1 root root 16 Aug  2 19:34 /usr/lib/llvm-4.0/lib/liblldb-4.0.so -> liblldb-4.0.so.1

I think it requires root access to dereference the symlink, so I kept this as lstat.

Also I've tried to merge the two .gyp, but to make it work I'll have to either

  1. migrate everything done in gyp_llnode to configure.js so node-gyp can build the plugin
  2. or get gyp_llnode does the work of node-gyp so it can build the addon

I prefer option 1. Either way I think it's best to leave the two gyp coexist in this PR for now and merge them later.

The tests now pass on Mac and Ubuntu with this command:

LLNODE_CORE=core.inspect.8.9.0 LLNODE_NODE_EXE=node.linux.8.9.0  node test/jsapi-test.js

Where node.linux.8.9.0 is the node-v8.9.0 binary and core.inspect.8.9.0 is the coredump taken from running that binary with test/fixtures/inspect-scenario.js. To make it work properly with the save core and test case I would like to wait for #144 to land first and reuse the code from there.

@bnoordhuis I think I've addressed all the comments, Can you take a look at the non-tests part again? Thanks!

joyeecheung

This comment was marked as off-topic.

@joyeecheung
Copy link
Member Author

Rebased now that the Travis CI is working. Fixed the syntax and API incompatibility on older versions of V8, also added -std=c++11 for older versions of node-gyp.

@bnoordhuis Can you take a look again? Thanks!

@joyeecheung
Copy link
Member Author

BTW: CI is green https://travis-ci.org/nodejs/llnode/builds/317264211 not sure why the bot does not updating the status in this PR though..

@joyeecheung
Copy link
Member Author

Rebased. Ping @bnoordhuis can you review again? Thanks!

mmarchini

This comment was marked as off-topic.

hekike

This comment was marked as off-topic.

@joyeecheung
Copy link
Member Author

I have a few more ideas about this after JSConf EU:

  • Use N-API now that we dropped support for Node 4
  • Expose the lists in streams
  • Expose the data as structured data so we will have object streams. This would make it easier to develop tools on top of the API

@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 8, 2018

So I've been working on the things I listed in #147 (comment) , my current design is to spit out everything that we know about the V8 values into JS and let the JS reconstruct objects based on existing knowledge when the users want it to (e.g. by default we return properties and elements of a JSObject, if the user just wants us to return something similar to what it used to be, we construct a new object out of those properties and elements).

But it turns out wrapping the V8 values into JS objects is quite a big undertaking that should probably be done in multiple future PRs, because we need to basically reverse all existing code in llv8.h and llv8-inl.h and write a formatter for each type of values.

Also the stream idea is less attractive if we use push streams i.e. Node.js streams. Pull streams seem to make a bit more sense but we'd either need to wait for the new Stream API or add a dependency like https://github.com/pull-stream/pull-stream . Either way the current iterator API already works and it should be trivial to implement a pull stream using that iterator.

So I think I will just finish the port to N-API and make it the first version of the API ready to land. The current API is not very good if you want to implement any GUI with it since it just gives you a bunch of strings (results ofInspect) that are supposed to be printed to the lldb console, but it just takes some grunt work to put the information into consumable JS objects nicely.

@joyeecheung
Copy link
Member Author

Superseded by #206

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 this pull request may close these issues.

5 participants