dunglas / frankenphp

🧟 The modern PHP app server
https://frankenphp.dev
MIT License
6.96k stars 244 forks source link

Add new build tag: newworker #1186

Open withinboredom opened 22 hours ago

withinboredom commented 22 hours ago

This is the bare minimum to allow compilation that removes worker.go by adding a new build tag: newworker.

This allows us to refactor and merge major changes to workers without breaking existing workers and work together by merging "dead code". It also defines NEW_WORKER in C, so that we can do #ifdef's depending on the worker running.

Once we are ready, we can delete the build tag and "old workers", making "new workers" the default implementation.

see: #1175

dunglas commented 21 hours ago

Couldn't we just use a new branch where we'll merge the related PR and rebase regularly against main? I'm not fond of having both implementations in the same code base.

withinboredom commented 21 hours ago

We could. However, looking at pros/cons:

Build tags

pros:

cons:

multiple branches

pros:

cons:

For a small team, on a young codebase (where a bugfix can potentially end up with a refactor), I'd suggest the build tag approach so that both implementations can be changed together. For a bigger team, or a more mature codebase, I'd suggest the branch approach.

AlliBalliBaba commented 11 hours ago

can make drastic, sweeping changes

Fixing fibers and allowing dynamically starting/stopping workers also requires a lot of changes in frankenphp.c and other files. That makes keeping around both implementations too messy IMO

withinboredom commented 10 hours ago

Less messy than doing a complex rebase, IMHO.