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

fix(command-dev): replace deprecated static-server dep by fastify-static #5341

Merged
merged 6 commits into from
Jan 3, 2023

Conversation

tinfoil-knight
Copy link
Contributor

@tinfoil-knight tinfoil-knight commented Dec 31, 2022

πŸŽ‰ Thanks for submitting a pull request! πŸŽ‰

Summary

Fixes #4511

The static-server dependency which is currently being used by the netlify dev command is now deprecated. This PR intends to replace the dependency with fastify-static (recommended by @lukasholzer).


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’». This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)
πŸ¦–

@tinfoil-knight tinfoil-knight self-assigned this Dec 31, 2022
@tinfoil-knight tinfoil-knight added type: security code to address security issues area: command: dev labels Dec 31, 2022
@github-actions
Copy link

github-actions bot commented Dec 31, 2022

πŸ“Š Benchmark results

Comparing with e1f661a

Package size: 259 MB

⬆️ 6.60% increase vs. e1f661a

^                                                                                                  259 MB 
β”‚  249 MB  249 MB  249 MB  249 MB  249 MB  249 MB  249 MB  249 MB  249 MB  249 MB  249 MB  242 MB   β”Œβ”€β”€β”  
β”‚ β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€|β–’β–’|──
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@tinfoil-knight tinfoil-knight force-pushed the 4511-remove-static-server-dep branch 2 times, most recently from 48c7005 to d83296e Compare December 31, 2022 14:20
@tinfoil-knight
Copy link
Contributor Author

tinfoil-knight commented Dec 31, 2022

Change in Response Headers

  • For successful request OR not found requests with 404.html file present:
    cache-control (public, max-age=0) is added.

  • For not found requests without any 404.html file:
    transfer-encoding (chunked) is not there now.

  • content-type for responses with a file has changed from text/html to text/html; charset=UTF-8


Note
In production Netlify, cache-control is public, max-age=0, must-revalidate.

const rootPath = path.resolve(settings.dist)
server.register(fastifyStatic, {
root: rootPath,
etag: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ETag generation is disabled since the proxy already handles that.

})

server.addHook('onRequest', (req, reply, done) => {
reply.header('X-Powered-by', 'netlify-dev')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name: 'netlify-dev' configuration in static-server used to set the X-Powered-by header which is now being done manually here.

@tinfoil-knight tinfoil-knight marked this pull request as ready for review December 31, 2022 20:32
danez
danez previously approved these changes Jan 3, 2023
Copy link
Contributor

@danez danez left a comment

Choose a reason for hiding this comment

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

Awesome. Happy new year.
I was already thinking about doing this. and even replacing express everywhere, but that would be a much bigger task I think.

@danez danez added the automerge Add to Kodiak auto merge queue label Jan 3, 2023
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Jan 3, 2023

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@kodiakhq kodiakhq bot removed the automerge Add to Kodiak auto merge queue label Jan 3, 2023
@danez danez force-pushed the 4511-remove-static-server-dep branch from 51f163e to f62b4f4 Compare January 3, 2023 11:32
@danez danez added the automerge Add to Kodiak auto merge queue label Jan 3, 2023
@tinfoil-knight
Copy link
Contributor Author

Awesome. Happy new year.

Happy new year to you too @danez. It was fun working w/ you & other folks on the Netlify team past year.

@kodiakhq kodiakhq bot merged commit 2994d3e into netlify:main Jan 3, 2023
@tinfoil-knight tinfoil-knight deleted the 4511-remove-static-server-dep branch January 3, 2023 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: command: dev automerge Add to Kodiak auto merge queue type: security code to address security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore(deps): remove usage of the static-server dependency
2 participants