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

Internalize ecstatic library #631

Closed
wants to merge 9 commits into from
Closed

Internalize ecstatic library #631

wants to merge 9 commits into from

Conversation

thornjad
Copy link
Member

@thornjad thornjad commented May 26, 2020

Ecstatic is no longer maintained, and so we cannot rely on any future updates. This PR internalizes it as the core module.

This PR also pulls in all of ecstatic's tests so we don't lose that coverage. Unfortunately these were written with tap while http-server's tests are written with vows. I don't have a strong preference for one over the other, but these need to be reconciled before this PR can be merged. I'm leaning toward converting to tap because that means rewriting only one file.

TODO:

  • Consolidate test files to use either tap or vows, or both.
  • Ensure all tests run as expected

Fixes #568

@thornjad thornjad added this to the v0.13.0 milestone May 26, 2020
@thornjad thornjad added high priority Very important bug or security fix major version Major, potentially breaking, change dependencies Pull requests that update a dependency file labels Jul 16, 2020
@reergymerej
Copy link

In the interim, what about a minor version bump in the dependencies to sidestep a security concern?

https://snyk.io/test/npm/ecstatic/3.3.2

Remediation: Upgrade to ecstatic@4.1.4.

@willeastcott
Copy link

The PlayCanvas Engine repo has had a Dependabot warning about this for maybe 9 months now. I guess I could just patch the package-lock.json. But yeah, would be great if http-server bumped its ecstatic dependency to 4.1.3, as @reergymerej suggests. Any objections to me submitting a PR for that?

@thornjad
Copy link
Member Author

thornjad commented Feb 1, 2021

Sorry I got pulled away into other life things, I need to get back to this PR. Unfortunately ecstatic v4 has some major incompatibilities with http-server (a product of the two projects being far too closely coupled for so long), so simply bumping the dependency does not work. The goal of this PR is to remove the dependency entirely, fix the vulnerability internally, and go from there.

This PR just needs functioning tests right now if anyone has the time to take a look at them, I don't right now but will get back to it when I can.

@kimegede
Copy link

Hi @thornjad, is there something you will need help with, then I will like to help out :)

@zommerfelds zommerfelds mentioned this pull request Apr 29, 2021
@zbynek
Copy link
Contributor

zbynek commented Jun 28, 2021

@thornjad is this just about fixing the failing tests listed here https://travis-ci.org/github/http-party/http-server/jobs/762178038 ? If someone submitted a PR with a fix, what are the chances any maintainer would look at it?

@thornjad
Copy link
Member Author

thornjad commented Jul 1, 2021

Yeah tests passing is the only thing holding this up. I just haven't been able to find any time, a PR would definitely be welcomed and looked at.

@zbynek zbynek mentioned this pull request Jul 3, 2021
2 tasks
@thornjad
Copy link
Member Author

thornjad commented Jul 6, 2021

Closing this draft in favor of #693

@thornjad thornjad closed this Jul 6, 2021
@thornjad thornjad deleted the internalize-ecstatic branch August 9, 2021 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file high priority Very important bug or security fix major version Major, potentially breaking, change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation of ecstatic
5 participants