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

Use local-web-server package for all servers #473

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

camillobruni
Copy link
Contributor

@camillobruni camillobruni commented Jan 22, 2025

  • Merge the test, dev and npm run server commands to use local-web-server.
  • Send COEP headers by default
  • Run npm ci to regenerate package-lock and sort package.json entries
  • Resolve committed merge conflict in bundled news-site workload

Copy link

netlify bot commented Jan 22, 2025

Deploy Preview for webkit-speedometer ready!

Name Link
🔨 Latest commit cf0f4a4
🔍 Latest deploy log https://app.netlify.com/sites/webkit-speedometer/deploys/67c8b1f78fa2ef00088d286f
😎 Deploy Preview https://deploy-preview-473--webkit-speedometer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

fix
@camillobruni camillobruni requested a review from julienw January 23, 2025 13:55
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me!
Here are some suggestions, tell me what you think!

@julienw
Copy link
Contributor

julienw commented Jan 24, 2025

Looks like the CI failed too

- await server startup status
- enable logging
@camillobruni camillobruni added the trivial change A change that doesn't affect benchmark results label Feb 27, 2025
@camillobruni
Copy link
Contributor Author

I've just noticed that we changed the default port from 7000 to 8000 previously. Are folks ok with this in general? (No strong opinion from my side)

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me, thanks !

I just think you forgot to commit the new version of package-lock.json after the latest changes to package.json, so please do that before landing :-)
Thanks!

@julienw
Copy link
Contributor

julienw commented Mar 4, 2025

image
Also it's worth updating to latest version of koa (probably you can checkout the package-lock.json from the main branch, and run npm i again).

@julienw
Copy link
Contributor

julienw commented Mar 4, 2025

Also it's worth updating to latest version of koa (probably you can checkout the package-lock.json from the main branch, and run npm i again).

BTW I still see 2.15.3 instead of 2.15.4.

@camillobruni
Copy link
Contributor Author

I never know what the command for this is npm i didn't do the trick, so I now ran npm audit fix and updated a bunch of other packages too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trivial change A change that doesn't affect benchmark results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants