Skip to content

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