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

Ecstatic with tests #693

Merged
merged 17 commits into from
Jul 16, 2021
Merged

Ecstatic with tests #693

merged 17 commits into from
Jul 16, 2021

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Jul 1, 2021

Please ensure that your pull request fulfills these requirements:

  • The pull request is being made against the master branch
  • Tests for the changes have been added (for bug fixes / features)

What is the purpose of this pull request?

Remove dependency on ecstatic -- see #631 (this is based on @thornjad 's work, only the test code was fixed here + hack from #569 replaced by a nicer fix inside Ecstatic)

Fixes #568, fixes #518, fixes #35, fixes #276, fixes #689

@zbynek zbynek marked this pull request as draft July 1, 2021 23:31
@zbynek
Copy link
Contributor Author

zbynek commented Jul 3, 2021

@thornjad all green 🎉 https://travis-ci.com/github/zbynek/http-server/builds/231702777

I dropped Node 6 from the list of CI tested platforms.

@zbynek zbynek marked this pull request as ready for review July 3, 2021 15:49
@zbynek zbynek force-pushed the ecstatic-with-tests branch from b20fd41 to 3fbb8fe Compare July 3, 2021 18:10
@thornjad thornjad self-requested a review July 6, 2021 19:49
@thornjad
Copy link
Member

thornjad commented Jul 6, 2021

🎉 thank you so much for the PR! I'll fit in looking over it whenever I get chances.

For the hack from #569, do you know where the fix inside ecstatic is? I don't remember hearing anything about it, but then again, ecstatic has gone through some drama that drowned out everything else.

@zbynek
Copy link
Contributor Author

zbynek commented Jul 6, 2021

For #569 I've added the replacement into decodePathname directly. I think this way it allows you to have \ in filenames on Linux/Mac (as long as you pass them as %5C in URL) and everything gets normalized to / on windows.

Before this change the tests were failing on Windows with infinite redirect (because the hack was only active when loading http-server, and the tests only load core). Now the tests are passing on Windows which suggests that the fix works, but please double check that it makes sense.

Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

I would normally complain about the --mimetypes addition, but I'll let that go since it's already implemented in ecstatic and we're just exposing it.

Other than that, it looks like we have a merge conflict, then I think this looks good.

@thornjad
Copy link
Member

For the conflict, I'd say just merge master, rm package-lock.json and npm i, that should give us a good package-lock again

@thornjad
Copy link
Member

Didn't realize I had push permissions on your fork! I just merged master in. I'll see if I can get travis hooked up again so we can see tests easily

@thornjad
Copy link
Member

Eh, looks like I don't have enough permissions to add the new Travis configuration (we were using a now-deprecated version)

@thornjad
Copy link
Member

Huh, turns out Travis is paid now. I guess I'll see what we can get from it in this PR, then move to something better before the 0.13 release.

On my latest merge, it looks like fsevents is doing some complaining on non-Mac hosts, do you have any insight on that? It runs fine on my local Ubuntu system, so I'm not able to repro

@zbynek
Copy link
Contributor Author

zbynek commented Jul 12, 2021

@thornjad travis is running on my branch. I changed the config to to use npm@7, it worked for all OS/Node combos except Win+Node 8: https://travis-ci.com/github/zbynek/http-server/builds/232553767

The last commit removes this combo from test matrix, I didn't manage to test it on Travis though (ran out of the free credits...), maybe you have more luck with your account.

@zbynek
Copy link
Contributor Author

zbynek commented Jul 15, 2021

@thornjad do you want to merge as is or do you want to figure out free CI first?

@thornjad
Copy link
Member

Eh, sorry yeah CI is the issue, I've run out of credits on Travis too. I'm thinking maybe yeah I'll just merge and figure out CI before releasing to npm

@thornjad thornjad merged commit d6fa8dd into http-party:master Jul 16, 2021
@zbynek zbynek deleted the ecstatic-with-tests branch July 16, 2021 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants