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

Remove pagespeed module from our nginx container #1150

Merged
merged 8 commits into from
Jul 11, 2022

Conversation

jometzner
Copy link
Collaborator

@jometzner jometzner commented May 5, 2022

PR Type

[ ] Bugfix
[ X ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no API changes)
[ ] Build-related changes
[ ] CI-related changes
[ ] Documentation content changes
[ ] Application / infrastructure changes
[ ] Other:

What Is the Current Behavior?

Currently we use all sorts of filters included in the pagespeed module of NGINX. The contribution to actual page speed ranking is not 100% clear. Especially since the introduction of core web vitals it would be beneficial to re-evaluate the usage of this module.

Con List:

  1. Inactive community
  2. Old release
  3. Performance impact to core web vitals unclear
  4. Increases the burden on SSR process with periodic beacon calls
  5. Longer docker builds

What Is the New Behavior?

Does this PR Introduce a Breaking Change?

[ X ] Yes
[ ] No

Other Information

AB#76451

@dhhyi
Copy link
Collaborator

dhhyi commented May 5, 2022

@jometzner That was one of the first major deployment improvements when we initially set up the demo servers. And at the beginning it also yielded a highscore in performance. Of course a lot has changes since then and due to the added PWA complexity (and maintenance status recently shown by the pinning of a 2yo ubuntu base image) it might not be useful anymore.

I like the refactoring of using built-in nginx startup scripts. 👍

This might be the way to go for a maintainable PWA, still, I'm sad to see it go away 😭

@jometzner
Copy link
Collaborator Author

Well the module is not gone yet. We're re-evaluating the decision from the first days to see if it still fits the requirements. I can still remember that it did good on the page speed results.

nginx/Dockerfile Outdated Show resolved Hide resolved
@shauke
Copy link
Collaborator

shauke commented Jun 27, 2022

First local tests did not show any changes in the lighthouse performance results.
And it now works on M1 Macs. 👍
And it is mach faster too. So I am totally for it. 😉

@shauke shauke added this to the 3.0 milestone Jun 27, 2022
@jometzner
Copy link
Collaborator Author

You could give the pull request a thumbs up

@jometzner jometzner marked this pull request as ready for review June 27, 2022 14:26
@jometzner
Copy link
Collaborator Author

I'll write some documentation for this change before it gets merged

@jometzner jometzner marked this pull request as draft June 27, 2022 14:30
Copy link
Collaborator

@dhhyi dhhyi left a comment

Choose a reason for hiding this comment

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

Haven't tested in our project yet, will do that next.

nginx/Dockerfile Outdated Show resolved Hide resolved
nginx/docker-entrypoint.d/00-envcheck.sh Outdated Show resolved Hide resolved
@jometzner jometzner marked this pull request as ready for review July 1, 2022 12:30
@jometzner jometzner force-pushed the feature/remove-pagespeed-module branch from fab039e to c12db4b Compare July 1, 2022 20:02
@jometzner jometzner force-pushed the feature/remove-pagespeed-module branch from 12bf1e4 to a45edb0 Compare July 1, 2022 20:27
@jometzner
Copy link
Collaborator Author

I'll write some documentation for this change before it gets merged

dhhyi
dhhyi previously approved these changes Jul 4, 2022
dhhyi
dhhyi previously approved these changes Jul 6, 2022
@shauke shauke merged commit d2897f9 into develop Jul 11, 2022
@shauke shauke deleted the feature/remove-pagespeed-module branch July 11, 2022 09:38
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.

3 participants