Spomky-Labs / pwa-bundle

PHP library for generating a full featured PWA manifest
https://pwa.spomky-labs.com
MIT License
29 stars 1 forks source link

Some refactoring ideas #80

Closed tacman closed 2 months ago

tacman commented 3 months ago

Description

I think this should be call pwa-bundle, that than phpwa. It's clearer.

Since this is 6.4+, consider refactoring the bundle configuration to extend AbstractBundle. It's MUCH easier to work with. https://symfony.com/blog/new-in-symfony-6-1-simpler-bundle-extension-and-configuration

I was going to put something together for the PWA attribute discussed in #78 , but the old bundle configuration is so complicated that I gave up (you need to inject the controllers in the compiler pass to do the reflections and find the attributes).

In 6.4, you simply implement the CompilerPassInterface in the bundle class and add a process method. And boom, one of the most complicated concepts in Symfony works.

Obviously, it's a bonus for developers to have this, but does nothing for the developers using it, and it currently works, so I can see that there's not much impetus for doing it.

Example



final class SpomkyLabsPwaBundle extends AbstractBundle implements CompilerPassInterface()
{
    public function process()...
    public function load()
}
Spomky commented 2 months ago

Hi,

Thank you for the reference. I was aware of this and now the branch 1.1.x takes should be clearer.

I think this should be call pwa-bundle, that than phpwa. It's clearer.

Naming thinks... Indeed the package name is not related bundle. I prefer let it as it is for now.

tacman commented 2 months ago

The vast majority of Symfony bundles are have a -bundle suffix. Yes, many of them are wrappers around another package, but -bundle makes it clear that it install with Symfony.

Please reconsider -- phpwa doesn't reflect what this bundle does, and I think it'd be hard for people to find just searching github or packagist. And it only works with Symfony, so it's not just a PHP library creating a PWA.

github-actions[bot] commented 1 month ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.