Replace fmin package dependency with local code #56
Conversation
also remove rmsprop for now, and add start/top methods to graphs
* Use npm package name instead of relative path * Update rollup config
…est tools (#16) * fix deprecation warnings, upgrade dependencies, replace jshint and tape with biome and vitest * use different line endings * cleanup package.json * update version
|
what is the license/copyright of fmin? at least this needs a clear indication in the code that this is from another repository. |
|
can you elaborate why this approach is better than updating fmin to the latest version only? shouldn't tree-shaking handle removal of all the unused code? |
|
it seems the fmin is licensed under BSD-3 while this package is MIT. |
|
@rwillingeprins have you thought of forking fmin, cleaning it up and pubhsling the fork on maintained fork on NPM? the this repo can just reference that one instead of getting into a grey zone of copying code. |
I think you already noticed it yourself but there was an error in the latest version of fmin as well as some remaining bloat.
Well even though this package is published with a single bundled file including the (partially) filtered dependencies, those dependencies (and their dependencies) are all still installed into node_modules because they are also listed in package.json. Would it make sense to list them under devDependencies instead if they are only needed during development to build the published venn.js?
I must confess I did not consider the license. My naive thinking was that both packages were authored by the same person and so if I kept the original commit history it would be clear enough who wrote the code.
I have no experience with publishing NPM packages but I would consider it if I actually used it directly. Right now though I haven't even looked into what the fmin code does exactly and I can't justify putting much more time into it. |
|
One way to address your concerns about the license mismatch might be to include the license from the fmin repo in this PR's fmin directory. Perhaps with a note in the main license file of this repo as suggested here. If you dont like that idea it might make sense for you to fork and publish it under the same @upsetjs scope as this package. I would be happy to help with that if I can. Otherwise we can hope that @benfred is willing to accept benfred/fmin#17 and publish a patch. |
|
@sgratzl, shall I just close this then? |
|
closing in favor of #57 which patches and inlines fmin in the build |
Moved only the relevant code of the fmin dependency directly into this repo with minor fixes. Besides reducing bloat, this fixes a high severity vulnerability and a couple of deprecation warnings.