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

add ManifestBuilder events, #173 #183

Open
wants to merge 1 commit into
base: 1.2.x
Choose a base branch
from

Conversation

tacman
Copy link
Contributor

@tacman tacman commented Apr 23, 2024

Target branch: 1.2.x
Resolves issue #173

  • It is a Bug fix
  • [ X] It is a New feature
  • Breaks BC
  • Includes Deprecations

Dispatches events during the manifest build step. For example, if there's no description in pwa.yaml, get it from composer.json

final class PwaConfigListener
{
    #[AsEventListener(event: PreManifestBuildEvent::class)]
    public function onPreBuildManifest($event): void
    {
        $config = $event->getConfig();
        if (empty($config['description'])) {
            $composerData = json_decode(file_get_contents(__DIR__.'/../../composer.json'), true);
            $config['description'] = $composerData['description']??'missing description!';
            $event->setConfig($config);
        }
    }

@Spomky
Copy link
Member

Spomky commented Apr 23, 2024

Hello @tacman,

I am not in favor of an event that could alter the configuration.
It may lead to undesired behavior.
Also, it is possible to tweak the configuration using env vars or have a bundle extension that implements PrependExtensionInterface.

@tacman
Copy link
Contributor Author

tacman commented Apr 23, 2024

Hmm. I quite like the idea of an event altering the configuration.

I know I can write a script that literally alters the pwa file, but because Symfony's YAML parser removes comments, I didn't like that approach. Plus, I'd have to constantly rerun it.

I've temporarily giving up on a different manifest depending on the subdomain, but I have some ideas. But for things like getting the name / description / screenshots, I was indeed hoping to intercept the Manifest Builder to make adjustments.

What undesired behavior are you concerned about, other than BC breaks and bugs the application developer may introduce?

@Spomky
Copy link
Member

Spomky commented Apr 25, 2024

Hmm. I quite like the idea of an event altering the configuration.

The problem is that you bypass the configuration validation and there are other means to tweak it (prepending the extension is the way I like).

I am wondering if the URLs (paths/params) could also by translated.
Not only for screenshots ('app.screenshot.1' => 'assets/screenshots/landing_page.png'), but also e.g. links for shortcuts (at least the _locale parameter). Not an easy task, but could solve use cases

@tacman
Copy link
Contributor Author

tacman commented Apr 25, 2024 via email

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

Successfully merging this pull request may close these issues.

2 participants