search

AlexTrotsenko / j2v8-debugger

This project is an add-on for the excellent J2V8 Project. It allows users to debug JS running in V8 using Chrome DevTools. Uses Stetho for communication with Chrome DevTools.
88 stars 24 forks source link

Use inspector #7

Closed jamie-houston closed 4 years ago

jamie-houston commented 4 years ago

This updates J2V8 to 6.1.0, and removes the DebugHandler for debugging. It uses the V8 Inspector instead.

jhorst11 commented 4 years ago

This resolves #6 🎉

AlexTrotsenko commented 4 years ago

@jamie-houston thanks for your MR I will check it in the coming days.

jamie-houston commented 4 years ago

@AlexTrotsenko thank you for taking the time to review this. At Salesforce, we are looking to make a significant volume of contributions back to this great project, to make it suitable for our own use. To do that, we'll need to be working in pretty quick iterations. Would you prefer to support us doing that in this repo directly, or should we look to make a distinct fork of the project so we can move at a more rapid pace?

AlexTrotsenko commented 4 years ago

@jamie-houston thanks for info regarding your needs and plans. I am thinking what could be the best for community. Right now given that some people using this repo already - let's keep the work here.

If you will find, that you need faster review circle - I would be happy to consider moving development to another fork.

P.S. indeed I was more busy than usual with some other projects these days, but I should look at this MR in the comming days.

AlexTrotsenko commented 4 years ago

@jamie-houston I did quick look through the MR and I like it overall.

just in case I will check-out it, build & run tests before merging .

Meanwhile I would like to ask you why have you used mapper in some cases and JSONObject directly in another? E.g. https://github.com/AlexTrotsenko/j2v8-debugger/pull/7/files#diff-2d434f41e408b47193a0b5278d195755R62

jamie-houston commented 4 years ago

Meanwhile I would like to ask you why have you used mapper in some cases and JSONObject directly in another? E.g. https://github.com/AlexTrotsenko/j2v8-debugger/pull/7/files#diff-2d434f41e408b47193a0b5278d195755R62

@AlexTrotsenko I'm not sure the reason I didn't use the mapper in some places. It might've been due to what changes I was making in the JSON and which seemed simpler. I can convert to use the mapper if you'd prefer.

I also realized this week that I hadn't updated the logic to handle multiple scripts... (as your original code did). In this PR it's only allowing a single one. I have a branch I'm almost done with that fixes that. https://github.com/jamie-houston/j2v8-debugger/pull/7 I can look at cleaning up the mapping logic in there as well. This PR also fixes some broken tests I missed.

I was planning to clean up/refactor the logic in onResponse in a future PR but can tackle that now since it'll tackle the mapping at the same time.

AlexTrotsenko commented 4 years ago

@jamie-houston if it's fine for you - let's have it included in this MR. Otherwise we can include it in the next one.

jamie-houston commented 4 years ago

@AlexTrotsenko I'll update it and ping you again when it's ready. Thanks!

jamie-houston commented 4 years ago

@AlexTrotsenko I've updated this PR with all of the changes, including fixing multiple scripts and all tests working.

AlexTrotsenko commented 4 years ago

@jamie-houston thanks for taking your time and updating this MR. I will check it in the next couple of days

jamie-houston commented 4 years ago

@AlexTrotsenko I've addressed all of the feedback. Let me know if there's anything else. Thanks for the thorough review.

AlexTrotsenko commented 4 years ago

Thanks for implementing the changes.

I have released 0.2.0, it's available in JitPack already: https://jitpack.io/#AlexTrotsenko/j2v8-debugger/0.2.0