Skip to content
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

feat!: recode in typescript #5

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

Bas950
Copy link
Contributor

@Bas950 Bas950 commented Sep 28, 2024

I started a recode of your monorepo using modern tools, like Vite, Vitest, and Typescript.

Package Status
@iplookup/country Finished and Tested
@iplookup/country-extra Finished and Tested
@iplookup/geocode Finished and Tested
@iplookup/geocode-extra Finished and Tested
ip-location-api Finished and Tested
@iplookup/util Finished and Tested

TODO:

  • Test custom license key

If you want, you can check out the branch, and feel free to leave some feedback!

@Bas950 Bas950 force-pushed the typescript-recode branch 2 times, most recently from b52bc35 to 40c7ac6 Compare October 3, 2024 12:51
@Bas950 Bas950 force-pushed the typescript-recode branch from 60b1738 to 06c10ab Compare October 12, 2024 11:07
@Bas950 Bas950 force-pushed the typescript-recode branch from 55111c7 to 8329375 Compare November 8, 2024 19:25
@Bas950
Copy link
Contributor Author

Bas950 commented Nov 9, 2024

Hey @sapics,

Could you take a look at it locally, and see if everything works like you expect it to?
I recoded the whole repo, but the core functionality has stayed the same.

Currently, all packages have tests and work.
The only thing that needs testing is a custom licenseKey in the ip-location-api package.
And I still need to add back all the debug logs.

To test it out:

  • Open the pull request branch locally
  • Run pnpm install
  • Run pnpm build
  • Run pnpm test:ui to verify everything works

Let me know if you spot any issues or if there's anything else you'd like me to adjust.

@Bas950 Bas950 marked this pull request as ready for review November 9, 2024 21:32
@Bas950 Bas950 force-pushed the typescript-recode branch from 92fd1b6 to 2ef21e5 Compare November 9, 2024 21:34
@Bas950 Bas950 changed the title feat: recode in typescript feat!: recode in typescript Nov 9, 2024
@sapics
Copy link
Owner

sapics commented Nov 11, 2024

Thank you @Bas950 for great works!
I am looking into it!

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 11, 2024

Thank you @Bas950 for great works! I am looking into it!
Thank you!

I will fix the failing tests later today or tomorrow.
And the conflicts with
browser/country-extra/package.json
browser/country/package.json
will re-occur daily because of the automatic updates.

In my version this won't occur anymore as these versions are published to NPM but not to GitHub.

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 12, 2024

@sapics Could you test with a customlicenseKey, my free one is not valid?

@sapics
Copy link
Owner

sapics commented Nov 13, 2024

Thanks @Bas950 for your great work!
It looks better than my codes!

However I cannot understand well how to work ;)
It is difficult for me to support new package system, therefore, I consider that it would be better to make new repository in https://github.com/Bas950 with new npm packages.
When you create new repository and v1.0 is published, I would like to close this repository and I make a link to your repository.
(ex. Please look at https://github.com/sapics/geoip-country as example which is the previous work of ip-location-api)

I guess that some users need to change codes after this PR, thus above method looks better for users also (ex. users need to change urls for browser version).

Are these proposals acceptable for you?

@sapics
Copy link
Owner

sapics commented Nov 13, 2024

Could you test with a custom licenseKey, my free one is not valid?

Sorry, I cannot understand well about your system, thus, I cannot make a test to create database.

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 13, 2024

Hey @sapics,

I would love to help maintain or even take over the package, but taking it over from you may be difficult as I don't have a MaxMind account/token. So maintaining it would be difficult as I can't check compatibility with MaxMind myself.

If you prefer, I can walkthrough the code with you, so I can explain what I changed.
And correct me if I am wrong, it seems like you haven't used these "modern" tools before, so I get it my seem daunting in comparison to plain/vanilla JS. But I am sure we can figure something out!

Let me know how you feel about it.

@sapics
Copy link
Owner

sapics commented Nov 14, 2024

I have only free account too. And I also could not success to download with free account.
The error is in sha256 checking, and it would work without error when I make a change to downloadAndExtractDatabase.ts as

- const shaText = await ky.get(shaUrl).text()
+ const shaText = await ky(shaUrl).text()

Could you try with this change?

@sapics
Copy link
Owner

sapics commented Nov 14, 2024

I understand roughly how it works, and I think it is possible to respond to other people's PRs and issues as they arise.

However, I guess that there are some problems with operating in this repository.

  • As you pointed out, I am not familiar with "modern style" (especially for typescript) and will likely mess up your clean code when I modify it.
  • I have not built it myself, so there is a risk that it will break in unexpected places after modification.
  • I don't know how to operate the npm package (especially best practice for typescript), since I don't understand the build process what you create.

Therefore, I consider that maintained in you repository would be better option for everyone.

( Sorry, I am not good at English, thus, I use translation site ;) )

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 15, 2024

I have only free account too. And I also could not success to download with free account. The error is in sha256 checking, and it would work without error when I make a change to downloadAndExtractDatabase.ts as

- const shaText = await ky.get(shaUrl).text()
+ const shaText = await ky(shaUrl).text()

Could you try with this change?

It actually works fine, I emailed MaxMind, I had a wrong type of licenseKey

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 16, 2024

Therefore, I consider that maintained in you repository would be better option for everyone.

I have made an organization (https://github.com/ip-lookup).
If you can join the organization and transfer this repository there. That way, the original repository is transferred.

If we do the same thing on NPM by adding me as a maintainer on the ip-location-api package (https://www.npmjs.com/package/ip-location-api/access) and on the organization (https://www.npmjs.com/settings/iplookup/members), that way I can update the original package with the new version, so users only have to update and not install a new package.

I will probably remake the redistribution (https://github.com/sapics/node-geolite2-redist) on that organization as well, so we have all the IP lookup repositories in 1 place.

@sapics
Copy link
Owner

sapics commented Nov 17, 2024

I feels few benefits to transfer the packages and repository.
To avoid troubles, it would be better to create new package and repository in my opinion.

npm package

The most of @iplookup users is just using url "https://cdn.jsdelivr.net/npm/@iplookup/country/iplookup.min.js".
Your url would be "https://cdn.jsdelivr.net/npm/@iplookup/country/dist/index.min.js".
I guess that most of users are not using versioning.
And database directory would be changed too, therefore, it would be trouble at updating to your new system.
(it makes trouble because we cannot know jsdelivr cache update timing)

repository

Your ip-location-api is basically the same code as mine, but the structure itself has changed so much that I feel that override committing to my code would only increase the git size and get in the way.
I still don't understand well your code, I guess there are some differences between my codes and your codes for how to use.
I believe that it would be more developmental if we develop the system according to your build system instead of according to my system for everyone.
Therefore, I think it is "not" necessary to follow my usage, so I think it is better to consider them separately.

Also, I would like to keep this repository in its current state if possible, as it cannot be undone once transfer is executed.

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 17, 2024

Alright, I see the issues.
I will make new packages for it then, I had a few other improvements in mind which I will go ahead and do as well. As backwards compatibility won't be the main focus then.

I will probably start work on it all this coming week.
I have made a new NPM organization for it too (@ip-lookup) so all the package names stay almost the same, but won't break any current stuff. (only a - got added)

And in preparation of supporting multiple DB providers like suggested in #13, I will probably rename the main package to @ip-lookup/maxmind, that way we can add other providers easily too.

I will keep in touch with you, if you don't mind, so we can insure a smooth transition for users of the package, and add clear deprecation warnings to the old ones. I will also be sure to give the appropriate credits to you as the creator of the packages, and that my maintained version is a continuation of yours.

@sapics
Copy link
Owner

sapics commented Nov 18, 2024

And in preparation of supporting multiple DB providers like suggested in #13, I will probably rename the main package to @ip-lookup/maxmind, that way we can add other providers easily too.

Looks great! Yours will get more users!

I will keep in touch with you, if you don't mind, so we can insure a smooth transition for users of the package, and add clear deprecation warnings to the old ones. I will also be sure to give the appropriate credits to you as the creator of the packages, and that my maintained version is a continuation of yours.

Thank you for your kind attention!

@Bas950
Copy link
Contributor Author

Bas950 commented Nov 29, 2024

Heya,

It’s been a while! I’ve been busy with work and took a short vacation, but I thought it was time to share an update.

Updates

I’ve created two new repositories:

The core, country, and geocode packages have already been transferred to the ip-lookup repository. However, I’ve decided not to transfer the -extra packages. These packages are rarely used and can easily be implemented by users themselves with about 10 lines of code. Instead, I propose we deprecate these packages and provide a clear migration guide. This guide will show users how to utilize the new package and add those lines manually if needed.

Geocode Updates

The geocode package has been updated to use MaxMind data, aligning it with the other packages. It no longer relies on DB-IP data. In the future, as previously discussed, we plan to add support for multiple database providers. I’ll explore extending this functionality to the country and geocode packages, allowing users to choose their preferred database provider.

To simplify updates, I plan to host and distribute the indexes for the country and geocode packages directly through the redistribution repository. This approach ensures the indexes are always up-to-date without relying on jsDelivr or requiring frequent package updates.

Do you agree with this approach for the country and geocode packages?

@sapics
Copy link
Owner

sapics commented Nov 29, 2024

I’ve decided not to transfer the -extra packages.

It makes sense. Looks good!

Geocode Updates

For browser system, each end user downloads database partially.
I was not employ MaxMind database for browser, because I was worried about MaxMind's EULA.

@Bas950
Copy link
Contributor Author

Bas950 commented Dec 2, 2024

For browser system, each end user downloads database partially. I was not employ MaxMind database for browser, because I was worried about MaxMind's EULA.

Exactly, each user will download it from, for example:
https://raw.githubusercontent.com/ip-lookup/redistribution/main/maxmind/indexes/country/4.idx
instead of
https://cdn.jsdelivr.net/npm/@ip-lookup/country/indexes/4.idx

That way they always have the latest version (to comply with privacy regulations).
And I think the EULA of MaxMind would work the same way for those indexes as the zips that are already redistributed there.

@Bas950
Copy link
Contributor Author

Bas950 commented Dec 5, 2024

For browser system, each end user downloads database partially. I was not employ MaxMind database for browser, because I was worried about MaxMind's EULA.

Exactly, each user will download it from, for example: https://raw.githubusercontent.com/ip-lookup/redistribution/main/maxmind/indexes/country/4.idx instead of https://cdn.jsdelivr.net/npm/@ip-lookup/country/indexes/4.idx

That way they always have the latest version (to comply with privacy regulations). And I think the EULA of MaxMind would work the same way for those indexes as the zips that are already redistributed there.

@sapics please correct me if I am wrong tho lol

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.

2 participants