Skip to content

fix deprecation warnings, upgrade dependencies, replace linting and test tools #16

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

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

lloydjatkinson
Copy link
Contributor

@lloydjatkinson lloydjatkinson commented Sep 12, 2024

Hi, I made this PR as I am getting various deprecation warnings for different dependencies and have been chasing those down and upgrading where I can.

In the process of this, I found that tape (used by this library) has a significant number of critical and high vulnerabilities coming from the packages it uses. In an effort to cleanup the ecosystem a little bit I took the liberty of replacing some of the packages that fmin uses while still maintaining the same behaviour.

jshint also has a similar situation and has been essentially replaced by other tools anyway.

I replaced tape with vitest (side note: after raising an issue on the tape repo it is clear they wish to remain in the past and are hostile towards dropping Node 0.10 support - everyone should migrate away from it) and jshint with biome. In the process I ran some linting and light formatting. There were several problems biome found that jshint does not, and where it makes sense I fixed those. There are some more errors which I have configured as warnings with a TODO comment.

The unit tests pass, the output from uglify-js and rollup (both also upgraded) is the same as before, and of course the actual algorithms remain unchanged (other than the linting and formatting).

Additionally, ES modules and syntax are now used.

@beatriz
Copy link

beatriz commented Sep 30, 2024

Hello! Is there any chance that this could be reviewed, merged, and released? It also solves the rollup high vulnerability, which is super useful for downstream projects.

@lloydjatkinson
Copy link
Contributor Author

I wish it would be but I believe that @benfred has no interest in further maintenance of several libraries or handing it over to someone who can. Maybe this PR should be a fork instead, just like had to happen with the venn.js library? https://github.com/upsetjs/venn.js

@beatriz
Copy link

beatriz commented Sep 30, 2024

If that is the case, I agree that a fork would be the way to go.

@beatriz
Copy link

beatriz commented Oct 2, 2024

@lloydjatkinson are you going to create a fork for this project with this PR? That would be super useful, but something I don't feel at all up to do as I am rather clueless on the workings of this project 🙂

@lloydjatkinson
Copy link
Contributor Author

lloydjatkinson commented Oct 7, 2024

Potentially - I keep hoping the original maintainer can merge it - the bulk of the work is done, and being able to push it to his original NPM package means everyone can get it, all that is required is @benfred to run npm publish. If I do make the fork, many downstream dependencies wouldn't even know the fork existed until they eventually come looking on this repo. If we can avoid making the split happen at all that would be good...

@santosh1994
Copy link

Potentially - I keep hoping the original maintainer can merge it - the bulk of the work is done, and being able to push it to his original NPM package means everyone can get it, all that is required is @benfred to run npm publish. If I do make the fork, many downstream dependencies wouldn't even know the fork existed until they eventually come looking on this repo. If we can avoid making the split happen at all that would be good...

If @benfred doesn’t respond within a certain timeframe, do you think it makes sense to go ahead and release the package? This way, we can specify the new package as a resolution to resolve dependencies, for example:

{
  "resolutions": {
    "package-name": "alternate-package@version"
  }
}

@benfred benfred merged commit 2d1df55 into benfred:master Oct 29, 2024
@benfred
Copy link
Owner

benfred commented Oct 29, 2024

thanks for the changes - pushed v0.0.4 to https://www.npmjs.com/package/fmin

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.

4 participants