Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Please release a minor version which replaces every occurrence of 'localhost' with '127.0.0.1' #486

Closed
vladejs opened this issue Oct 19, 2018 · 16 comments

Comments

@vladejs
Copy link

vladejs commented Oct 19, 2018

I don't know why, but having sapper in its source code localhost gives me trouble with my (custom) setup, specifically, the server can't start (it happens only on Windows, only on my custom project setup, the default sapper template runs fine 0_o).

Right now, If, on my project, I go to node_modules/sapper folder and replace every occurrence with '127.0.0.1' the problem gets solved.

It will be very helpful to me (and would harm no one) if you could release a minor version with these changes @Rich-Harris .

@Conduitry
Copy link
Member

Where are you changing localhost in the Sapper source? As far as I can tell, the only instances of it in this project are related to messages displayed in the console and the CLI's export feature. I can't see either of those having any effect on you.

The only other place I see localhost in the built package is coming from the bundled port-authority dependency. It's definitely possible that's involved.

If you could share a minimal Sapper project that demonstrates this, that would be appreciated - because I don't think we'll want to make this change without actually seeing what's going on.

@vladejs
Copy link
Author

vladejs commented Oct 20, 2018

Go to node_modules/sapper and do a fuzzy search on the word localhost, you will find several files on the dist folder that uses localhost in code.

Here is my fully customized setup:
https://github.com/vladejs/sapper-server-crash

The problem happens on my Windows 10 Home Edition
Node 10.9, but I also tested on node 8.x

@vladejs
Copy link
Author

vladejs commented Oct 27, 2018

Any news on this @Conduitry, @richharris?

@Conduitry
Copy link
Member

Sorry, I missed that you had included a repo before. I just tried this out locally, and I'm not seeing anything wrong. (Node 10, Windows 10 Pro.) As I said above, I was wary about making this change without some maintainer at least making an effort to reproduce this.

What specific error are you getting? As I said above, as far as I can tell, the only localhost in sapper's dist folder is that I can see affecting you at all is one that is actually coming from https://github.com/Rich-Harris/port-authority - and that one is only used to let you know when the sapper server has started.

@lukeed
Copy link
Member

lukeed commented Oct 27, 2018

We should read from process.env.HOST specifically for this case. TBH I thought we already did. This allows Windows users (who 9/10 have some VM going) to configure it as needed, and everyone else will continue to default to localhost.

Doesn't need minor, could be a patch

@Conduitry
Copy link
Member

@lukeed I'm not sure what you mean exactly, but what you're saying seems more relevant to #485 I think. I believe the only places that Sapper seems to hardcode localhost are - 1) in output messages 2) during sapper export and 3) via port-authority. In particular, the server is always bound to an unspecified interface.

@lukeed
Copy link
Member

lukeed commented Oct 27, 2018

Sure – I haven't looked into this carefully yet, but this one may be problematic when dealing with VMs.

@Conduitry
Copy link
Member

Ah I guess my brain parsed that one as console output to the user for some reason because it was in cli.ts. I don't think that instance alone would warrant a new env variable. Maybe an optional argument to the cli flag?

But either way, this doesn't seem related to whatever issue is happening in this ticket. I really wish I knew what exactly the error message was.

@vladejs
Copy link
Author

vladejs commented Oct 27, 2018 via email

@vladejs
Copy link
Author

vladejs commented Oct 27, 2018

@Conduitry?

@Conduitry
Copy link
Member

Your repo name is called 'server crash' - have you experienced any actual failures besides this message? I see occasional issues where I'm told the server is not listening on a port, while everything is fine. I'm thinking there's a race condition somewhere. We should probably get to the bottom of this eventually, but if that's all you're seeing, I don't think we want to just change a bunch of localhosts to 127.0.0.1s, which is unlikely to truly resolve it.

@vladejs
Copy link
Author

vladejs commented Oct 28, 2018 via email

@vladejs
Copy link
Author

vladejs commented Nov 20, 2018

Line 258: src/api/dev.ts
Replace that with a call to the same ports.wait fn, so it will retry.

@vladejs
Copy link
Author

vladejs commented Nov 29, 2018

No comments on this? Its a simple fix.
I also saw the last commit was done long ago.

Is this still maintained?

@Conduitry
Copy link
Member

If by "still maintained" you mean "the maintainers will make arbitrary changes that people ask them to, even if they don't see the benefits of them", then no the project is not maintained.

Is this still about replacing 'localhost' with '127.0.0.1'? That still seems to have no bearing on any actual problems.

Is this now about retrying the ports.wait call? This also doesn't feel like the correct solution. Have you tried increasing the timeout on the original ports.wait call? (See its API.) If that doesn't work, there may be an issue with that library - perhaps this timeout shouldn't be hardcoded.

@vladejs
Copy link
Author

vladejs commented Dec 6, 2018

By "still maintained" I meant the whole project since the last release was about a month ago and issues are piling up. I'm trying to probe my co-workers that this stack is better than anything React related, and I'm just worried.

I've tried increasing the timeout, it sooner or later fails. I spent whole days finding a solution and the retry one is the one that has worked fine.

I have seen with my local implementation up to 14 retries until it correctly connects.
I also think that a retriable mechanism increases the error tolerance of sapper against situations like this.

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

No branches or pull requests

3 participants