Skip to content

Internalize ecstatic library - #631

Closed
thornjad wants to merge 9 commits into
masterfrom
internalize-ecstatic
Closed

Internalize ecstatic library#631
thornjad wants to merge 9 commits into
masterfrom
internalize-ecstatic

Conversation

@thornjad

@thornjad thornjad commented May 26, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown

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
Copy Markdown

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

thornjad commented Feb 1, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown

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

zbynek commented Jun 28, 2021

Copy link
Copy Markdown
Contributor

@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

thornjad commented Jul 1, 2021

Copy link
Copy Markdown
Member Author

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

thornjad commented Jul 6, 2021

Copy link
Copy Markdown
Member Author

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