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

[Docs] Document server-side construct in Symfony docs #1014

Merged
merged 2 commits into from
Sep 16, 2021

Conversation

t-richard
Copy link
Member

This is a first shot at documenting the Lift server-side construct with Symfony.

Encore.setManifestKeyPrefix('build/')
}
```yaml
...
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a more complete example like in Lift docs ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think it could be worth it, because so far it could be confusing: "do I keep the lambda functions?" "where does this go in serverless.yml?", etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure how to tackle this. I don't want to put the whole serverless.yml either because that would mean having to maintain it in two places (here and the symfony/recipes-contrib repo). And that we tried to avoid in previous PRs like #923 (comment)

Maybe something like this would be a good compromise ? Really not sure, any suggestion appreciated 🙂

service: symfony

provider:
    ...

plugins:
    - ./vendor/bref/bref
    - serverless-lift

constructs:
    website:
        type: server-side-website
        assets:
            '/bundles/*': public/bundles
            '/build/*': public/build
            '/favicon.ico': public/favicon.ico
            '/robots.txt': public/robots.txt
            # add here any file or directory that needs to be served from S3

functions:
   ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is exactly what I had in mind.

Just to show familiar stuff first, I would put functions: above constructs:

@mnapoli
Copy link
Member

mnapoli commented Sep 7, 2021

That looks awesome! Thank you for getting this started.

Usually I'm not a fan of duplicating instructions between "Websites", "Laravel" and "Symfony", but that new construct makes it so simple that I'm fine with it :) Especially since the example can now contain the exact list of assets URL specifically for Symfony, that's awesome.

@t-richard t-richard marked this pull request as ready for review September 13, 2021 21:41
@t-richard
Copy link
Member Author

Forgot to mark this as ready when I updated code and docs last week.

Let me know if there's anything I can do to help move this and/or the broader "Website assets" docs forward

@mnapoli
Copy link
Member

mnapoli commented Sep 16, 2021

Oh cool, let's merge then, thanks a lot!

@mnapoli mnapoli merged commit d1dd690 into brefphp:master Sep 16, 2021
@mnapoli
Copy link
Member

mnapoli commented Sep 16, 2021

If you have time to add something similar to the rest of the documentation I'm all for it!

In the "Website assets" docs we can remove the huge YAML template, we don't need to document that anymore.

@t-richard
Copy link
Member Author

@mnapoli What's your vision on this page ? Should we keep the part with s3 on a separate domain ?

@t-richard t-richard deleted the document-symfony-website-construct branch September 16, 2021 16:02
@mnapoli
Copy link
Member

mnapoli commented Sep 17, 2021

TBH I'm not 100% sure.

Maybe something like this (broad outline):

That way the documentation stays simple, focused on one approach. Yet we offer alternatives and the existing documentation (with full examples) is not 100% lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants