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

Do I have permission to refactor this? #527

Open
oplanre opened this issue Feb 23, 2024 · 3 comments
Open

Do I have permission to refactor this? #527

oplanre opened this issue Feb 23, 2024 · 3 comments

Comments

@oplanre
Copy link

oplanre commented Feb 23, 2024

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
Copy link
Collaborator

@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
Copy link
Author

oplanre commented Feb 23, 2024

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
Copy link
Collaborator

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

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

No branches or pull requests

2 participants