phpv8 / v8js

V8 Javascript Engine for PHP — This PHP extension embeds the Google V8 Javascript Engine
http://pecl.php.net/package/v8js
MIT License
1.84k stars 200 forks source link

Do I have permission to refactor this? #527

Open oplanre opened 8 months ago

oplanre commented 8 months ago

I can bring this up to modern c++ standards, i just want to know if I have permission to, and that you would merge the request as long as you find no issues in the code (don't want to waste my time). Most of the c++ code will change but the userland api will remain backward compatible(for the most part e.g i am considering a V8 or V8JS namespace instead of the current global classes)

redbullmarky commented 8 months ago

@oplanre I don't run things around here to make the call, but we are (very!) heavy users of the library and push it maybe further than its original intent...so I have a couple of opinions.

My main question would be...to what end? I love refactoring code and often have to resist doing it purely for the sake of making it look nice, because even with all the care and best intentions in the world, refactoring an entire project is a big risk. The code in many places may be a little old, but it works (even on latest PHP & V8 versions), is reasonably easy to find your way around and performs really well. Now...if there are specific parts of the code where you're thinking..."this has GOT to be really inefficient" then that's a legitimate refactor. Not to mention the peer review on a smaller chunk of code is going to be more favourable than waiting a LONG time for the entire thing to be reviewed/tested etc :) Repeat that enough times on other parts of the code, with actual legitimate & measurable purposes in mind, and you're almost at a point where you've done what you set out to do anyway.

There are also a bunch of open issues; each of which could justify an element of refactoring AND also produces something positive from the change other than just nicer code. Perhaps those would be a good place to start?

I'm not sure you'd be able to get any kind of promise to merge an entire refactoring, though. Like I mentioned above - it's a huge review/testing job afterwards and without any actual idea of what the new code would look like, whether it's actually better/faster etc.

I'd certainly be pleased to see more contributors though, it's a great project that holds up REALLY well to everything we put it through and would love to see it grow further.

oplanre commented 8 months ago

Three main things that i would like to achieve:

  1. Cleaner buildsystem (A good chunk of issues are related to this)
  2. An api that can be exposed to other php extensions: We currently use FlatBuffers and a rough impl of nikic/buffer for (de)serializing data between php and v8. We have a simple CRTP base V8 object class for creating objects with custom handlers by overriding methods from the C++ side, works great
  3. At least a gen_stub.php implementation for the arginfos (this is important for the above point as it allows us to generate custom v8 objects automatically by writing stub php code).

Extra: We have an experimental A+ Promise impl with fibers, still early stages but i would like some extra eyes\input on it.

I understand that you can't promise to just accept my refactor upfront, I guess a better way to phrase it would be: Will you seriously consider my refactor or would I be better off with a fork?

Cheers, Lanre.

redbullmarky commented 8 months ago

Hey In my mind, "consider a refactor" or "better off with a fork" aren't mutually exclusive; in any case, you'd be forking the project, making the changes etc. The only thing then would just be creating a pull request - and whether that gets approved or not. I'd imagine if it got merged, you'd use this project - if not, you'd use your own.

Could parts (cleaner build, gen_stub, namespaces, basic non-breaking cleanup/preparation, etc) be done separately/initially ahead of a full refactor? I'd imagine those things would be beneficial even before making widespread changes. Breaking the work up into smaller chunks rather than a single, huge rebuild would certainly make further merges more likely, I'd imagine.

So overall:

  1. prep/organisation/cleanup/minor tweaks - things that don't fundamentally change much. Cleaning up, but otherwise leaving everything "recognisable" and easier to build/install.
  2. actual refactoring.

As for going that far though, I'd imagine @stesie would have an opinion or two there.

For use case context, one of our main approaches (rather than single-run-and-get-a-primitive-result) is to create object instances based on a user-generated JS "hooks" class within V8Js - that is then returned to PHP and the various methods (where provided) are executed when necessary. The overall V8Js instance (and the instance returned via an executeString) are kept alive as long as necessary.

Cheers Mark