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

upgrade nodes2ts dependency to 2.0.0 #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Bersaelor
Copy link

This should hopefully fix the problem in nodes2ts version 1.1.0 as described in #8 🤞

I'm relatively new to this library, wanted to use it in a AWS Lambda environment and encountered the linked issue too.

@Bersaelor
Copy link
Author

Bersaelor commented Oct 2, 2020

PS: I do applaud your effort in adding tests to this framework, made it much easier to upgrade nodes2ts dependency without knowing all the inner workings of dynamodb-geo.js

@Bersaelor
Copy link
Author

Unfortunately, I found another issue in nodes2ts v 2.0.0 : vekexasia/nodes2-ts#12

Please hold off on merging this yet.
Also, we might want to add an extra test, as this issue was not apparent from the tests, i.e. the method in GeoDataManager that led to the unimplemented method in nodes2ts being called, was not part of any of the tests in dynamodb-geo:

at Function.S2.exp (/var/task/node_modules/nodes2ts/dist/S2.js:48:15)
    at S2Metric.getMaxLevel (/var/task/node_modules/nodes2ts/dist/S2Metric.js:66:32)
    at S2RegionCoverer.getInitialCandidates (/var/task/node_modules/nodes2ts/dist/S2RegionCoverer.js:320:74)
    at S2RegionCoverer.getCoveringInternal (/var/task/node_modules/nodes2ts/dist/S2RegionCoverer.js:365:14)
    at S2RegionCoverer.getCoveringUnion (/var/task/node_modules/nodes2ts/dist/S2RegionCoverer.js:180:14)
    at S2RegionCoverer.getCoveringCells (/var/task/node_modules/nodes2ts/dist/S2RegionCoverer.js:155:24)
    at GeoDataManager.<anonymous> (/var/task/node_modules/dynamodb-geo-bersaelor-test/dist/GeoDataManager.js:253:94)
    at step (/var/task/node_modules/dynamodb-geo-bersaelor-test/dist/GeoDataManager.js:46:23)
    at Object.next (/var/task/node_modules/dynamodb-geo-bersaelor-test/dist/GeoDataManager.js:27:53)

@murbanowicz
Copy link

@rh389 Are you going to merge it or you're not putting any effort on this project anymore?
I am in front of making decision between Dynamo and Mongo and this library is basically leading my decision...

@robhogan
Copy link
Owner

robhogan commented Oct 7, 2020

@murbanowicz note the issue above and in particular:

Please hold off on merging this yet.

Also, if you're basing your choices on the maintenance of the library you should know that I'm personally no longer using it and maintainers are wanted (#45).

@Bersaelor
Copy link
Author

I have asked @vekexasia the maintainer of nodes2-ts what the plans are to fix the issue in the 2.0.0 version of nodes2-ts. Still waiting.

joenye pushed a commit to joenye/dynamodb-geo.js that referenced this pull request Oct 12, 2020
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.

3 participants