fix(deps): replace string-similarity with fastest-levenshtein #5759
Conversation
|
@kisaragi-hiu thanks for the PR! The changes look good to me, but i'd like to make sure that changing the algo to Levenshtein doesn't have unintended consequences. @lukasholzer I see you originally implemented the string similarity 2 years ago. Did you pick a sorensen-dice-based algorithm on purpose? Do you think we should stick with that, and maybe use a library like https://www.npmjs.com/package/dice-coefficient instead? Or is it fine to switch to Levenshtein? |
lukasholzer
left a comment
lukasholzer
left a comment
There was a problem hiding this comment.
@Skn0tt to be honest I cannot remember why I've decided to go along with the string-similarity package.
But using the fastest Levenshtein distance is probably a good decision as it measures the necessary edits so in this case it looks pretty well suited.
@kisaragi-hiu thanks for putting the effort into this! Looks good to me :)
Do you mind maybe adding a test to it?
kisaragi-hiu
force-pushed
the
replace-string-similarity
branch
from
225dc30 to
9b5faa8
Compare
string-similarity is no longer maintained and deprecated, resulting in another warning when installing Netlify CLI. While the algorithm is different (Levenshtein distance instead of Dice's coefficient), the difference shouldn't be big enough to matter as this is only used for suggestion after a typo.
kisaragi-hiu
force-pushed
the
replace-string-similarity
branch
from
9b5faa8 to
6cae00f
Compare
When the user specifies a recipe that doesn't exist, the recipes command is supposed to suggest a replacement. The way it detects this scenario is to catch the module not found error, but it was catching MODULE_NOT_FOUND, which is only thrown for CommonJS modules: > MODULE_NOT_FOUND > > A module file could not be resolved by the CommonJS modules loader > while attempting a require() operation or when loading the program entry > point. > > --- https://nodejs.org/api/errors.html#module_not_found For ESM modules, when a module isn't found, the ERR_MODULE_NOT_FOUND error is thrown instead: > ERR_MODULE_NOT_FOUND > > A module file could not be resolved by the ECMAScript modules loader > while attempting an import operation or when loading the program entry > point. > > --- https://nodejs.org/api/errors.html#err_module_not_found This commit makes it catch ERR_MODULE_NOT_FOUND instead.
kisaragi-hiu
force-pushed
the
replace-string-similarity
branch
from
6cae00f to
e35b7c0
Compare
|
I've added the tests for typo suggestion for both the recipes command and the main command. During this I discovered that the typo suggestion is broken for the recipes command, as it is catching MODULE_NOT_FOUND (thrown for CJS) and not ERR_MODULE_NOT_FOUND (thrown for ESM). I fixed it in the same commit as adding the recipes test --- should the fix be in a separate PR instead? I mistakenly pushed |
Skn0tt
left a comment
Skn0tt
left a comment
There was a problem hiding this comment.
looks great! thanks so much for this PR @kisaragi-hiu!
3 participants
Summary
Replace string-similarity with fastest-levenshtein as the former is deprecated, resulting in another warning during installation.
While the algorithm is different (Levenshtein distance instead of Dice's coefficient), the difference shouldn't be big enough to matter, as this is only used for suggestion after a typo.
Partially addresses #5724.
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.