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: revert changes that cause slow proxying #758

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

Conversation

johnnyreilly
Copy link

@johnnyreilly johnnyreilly commented Oct 24, 2023

This PR addresses #736 which concerns slow proxied requests for static content. These slow proxied requests were caused by the changes made by @cjk7989 in #720

Whilst well intended, these changes fixed one issue that presented with Node.js 18 but introduced a more problematic issue that made the SWA CLI difficult to use for local development where static assets are involved. This change was released with version 1.1.4. Speaking for my own organisation, we have pinned to 1.1.3. Looking at the feedback in #736 this seems a common approach.

Looking at the comments in #736 it's clear this is affecting many people. This PR reverts that change.

Motivations

  1. The implemented change has not resulted an improved development experience - rather it prevents people from using the SWA based upon personal experience and the feedback provided in Proxied requests for static content through the SWA CLI are very slow #736. @burkeholland reported he re-architected an application to workaround this.
  2. The motivation behind the changes in fix: support dev server on node 18 #720 was to resolve [start] Start command does not connect to web or api server on Node 18 #663. Those issues arose with early versions of Node.js 18 that were affecting wait-on. The original issue is now resolved with Node.js 20 according to @MikeMcC399 comments: wait-on failure for localhost of react-app Node.js 18 jeffbski/wait-on#137 (comment)
  3. Node.js 20 is now the LTS version of Node.js. The original issue that motivated the change which caused the regression, no longer applies if using LTS Node.js
  4. Having users pinned to 1.1.3 is not a long term solution - this, if merged, should resolve that.

Finally, I wanted to say a quick thank you! ❤️

I really the love the SWA CLI and use it in my work on a daily basis - thank you for the great tool that it is!

@cjk7989
Copy link
Contributor

cjk7989 commented Oct 24, 2023

Hi @johnnyreilly, thanks a lot for helping investigate and fix the issue : )

Instead of reverting, I'd like to test wait-on with node 18 and 20, and then submit a new PR, in order to ensure everything works as expected. I have also observed that some features may fail in Linux or MacOS with node 20, so the team may take time to decide whether and how to support node 20. I'll work on this soon. I look forward to discussing with you again.

@johnnyreilly
Copy link
Author

Thanks @cjk7989 - if it helps I work on both Linux and Mac and I'm happy to help with testing in that space.

@johnnyreilly johnnyreilly mentioned this pull request Oct 24, 2023
@itpropro
Copy link

itpropro commented Oct 30, 2023

We have also pinned 1.1.3, due to the extreme slowness and the difficulties this introduces for local development. I think this should be adjusted to work with the only still supported node versions 18 and 20 and should consider Linux, MacOS and Windows name resolution for localhost.
Node 16 is EOL and node 20 is the current LTS and should therefor considered as the default.

PS: Besides this current problem, the SWA CLI works great and is really enjoyable to work with.

@cjk7989
Copy link
Contributor

cjk7989 commented Oct 31, 2023

Hi @johnnyreilly, sorry for the delay of response. I have merged PR #761 to fix this issue. I hope this can help, though swa-cli will support node<=18 in a period of time. Could you test it if you are convenient? Thanks again for paying attention to this issue!

Testing steps if needed:

  1. git clone https://github.com/Azure/static-web-apps-cli.git
  2. cd into the folder "static-web-apps-cli"
  3. run "npm install", then "npm run build"
  4. run "npm link ./", which allows you to use this version of swa-cli globally.
  5. * if you want to update the version of swa-cli, just delete this local folder "static-web-apps-cli" and run "npm install -g @azure/static-web-apps-cli" to reinstall the official version.

@johnnyreilly
Copy link
Author

johnnyreilly commented Oct 31, 2023

Hey @cjk7989,

Thanks for this - I did some testing. It's better, but the news isn't great I'm afraid.

I took our most demanding client side app, and timed the initial startup with 1.1.3, 1.1.4 and 1.1.5-alpha for comparison. 1.1.5-alpha was half as performant as 1.1.3 - see the screenshots from devtools below:

1.1.3

image

1.1.4

image

1.1.5-alpha

image

Also with 1.1.5-alpha the terminal was filled with these messages:

[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173
[swa] [swa] Timed out waiting for: tcp:localhost:5173

@cjk7989
Copy link
Contributor

cjk7989 commented Nov 1, 2023

Hey @johnnyreilly, thanks a lot for testing! I will remove these timeout logs in swa-cli@1.1.5.

PS: Though remove wait-on before requesting to dev servers can be faster (as version 1.1.3 does), we can't do this, since we don't know if localhost will be resolved to ipv4 or ipv6. It depends on different framework (e.g. ipv6 for Angular, but ipv4 for React). Do you think the response speed of 1.1.5-alpha is acceptable for you?

@johnnyreilly
Copy link
Author

It's pretty slow - I'd stick with 1.1.3 rather than use 1.1.5

@johnnyreilly
Copy link
Author

we don't know if localhost will be resolved to ipv4 or ipv6. It depends on different framework (e.g. ipv6 for Angular, but ipv4 for React)

I'd be quite surprised if the front end framework code affected whether it was ip4 or ip6. Are you sure about that?

@cjk7989
Copy link
Contributor

cjk7989 commented Nov 2, 2023

I'd be quite surprised if the front end framework code affected whether it was ip4 or ip6. Are you sure about that?

Yes, when starting site localhost:4280, a request will be proxied to the dev server started by these frameworks. For example,
swa will ping [::1]:4200 for Angular, and ping 127.0.0.1:3000 for react. So we use wait-on to detect both IP address, which causes the response time to double.

@johnnyreilly
Copy link
Author

For example,
swa will ping [::1]:4200 for Angular, and ping 127.0.0.1:3000 for react. So we use wait-on to detect both IP address, which causes the response time to double.

I'm not sure it's quite like that? For instance, my app is a React app, but I use vite as the Dev server so it's http://localhost:5173

Is it not more down to the Dev server you're using? And couldn't we be specific and once the first request succeeds, store the successful responder and use that to steer all subsequent requests?

@cjk7989
Copy link
Contributor

cjk7989 commented Nov 7, 2023

Hi @johnnyreilly, we can't revert to 1.1.3 to speed up because the bug of resolving localhost will occur with node 18 in some systems. But I agree with you to record the successful responder and use that to steer all subsequent requests. We will design a fallback method to resolve requests in the future update. Thanks!

@johnnyreilly
Copy link
Author

Thanks @cjk7989! I'm going to pin to 1.1.3 for now, but I'd be more than happy to help with testing when you work on the fallback method.

By the way, if you ever want to have a call to discuss this, I'm open to it.

In our organisation we use the SWA CLI as a general purpose web development tool that allows us to avoid the hell of CORS. We use it when developing contain apps as well as static web apps

So we're highly invested in the success of this tool!

@itpropro
Copy link

itpropro commented Jan 2, 2024

First of all, happy new year!
@cjk7989 any updates on the CLI or the SWA runtime itself? Node 16 is eol since September and Node 20 is still not fully supported in SWA or the CLI.
The localhost problem is still existent and slows down the local dev experience.

@adrianhall
Copy link
Member

PR #862 updates wait-on (along with a slew of changes for CVEs, ES modules, and later versions of Node). Can you please check that out and see if it fixes your slowness issues as well?

@johnnyreilly I'd also like to chat with you on how you are using SWA CLI overall. If you are up for a live discussion on how you use SWA CLI, could you reach out via private email (adhal-at-microsoft) so we can set that up? Thanks!

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.

[start] Start command does not connect to web or api server on Node 18
4 participants