search

hayd / deno-udd

Update Deno Dependencies - update dependency urls to their latest published versions
MIT License
329 stars 18 forks source link

fix: npm registry with deno update syntax #109

Closed levibostian closed 9 months ago

levibostian commented 1 year ago

Fixes: https://github.com/hayd/deno-udd/issues/96

udd takes a npm import npm:ical-generator@^4.0.0 and converts that to npm:ical-generator@4.0.0#^ from this line of code

This pull request modifies the npm registry to behave like the other registries where it will remove the fragment piece from the string.

hayd commented 1 year ago

Thanks @levibostian , it certainly seems like udd is broken for the npm case so would be good to fix.

cc @Endy3032 @sigmaSd would this fix your issues too? (this seems smaller that #102)

sigmaSd commented 1 year ago

I don't remember the actual issues I had xD

But I remember that my PR main goal is to remove the custom logic of simver, which should make the codebase more readable, and have less edge cases that the current implementation

Though I understand its seems like a big change , and if things already work I guess there is no big motivation

sigmaSd commented 1 year ago

Now I'm getting back some memories, basically my personal opinion is such custom implementation will not scale, and will keep on acquiring fixes of edges cases with time, using deno info for structured deno and actual simver seemed like a better foundation

levibostian commented 1 year ago

I am not sure how similar this PR is to https://github.com/hayd/deno-udd/pull/102?

This PR fixes edge case bugs that occur while trying to use npm: syntax with udd. Since the syntax of npm: dependencies is different from your typical deno dependency using http:// URLs.

102 seems to modify the behavior of udd to using a different syntax for declaring semantic versioning.

I have personally come to like the design of udd and it's use of semantic versioning in the dependency string. However, npm: dependencies are a different syntax then the other repositories which complicates the current udd implementation. This bug fix took me quite a bit of time to figure out and feels like a hacky/fragile implementation, IMO.

To make it less fragile and hacky, I did consider a refactor. I like the idea of each repository class being in charge of parsing the dependency string, as it currently does, and do the responsibility of generating a Semver object that the mod module uses. As opposed to the mod module generating the Semver object as it does today.

Since each repository (deno land, npm) might have unique syntax and semantic version rules, I like the idea of the repositories having the code to do it. Which is a little of what I did in this PR by making the npm registry have a modified version() and at() function to behave more like the other repositories. I noticed that the npm registry version() function, for example, would return a version string with the #~ at the end of the version string but no other registry was doing that. So, I modified only the npm registry's version() function to behave similar to the other registries.

levibostian commented 9 months ago

Doing some PR cleanup. Stale, so closing.