Skip to content
This repository has been archived by the owner on Sep 30, 2019. It is now read-only.

Fix JavaMap .values() and .entrySet() #70

Merged
merged 3 commits into from
Jan 25, 2019
Merged

Fix JavaMap .values() and .entrySet() #70

merged 3 commits into from
Jan 25, 2019

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Jan 23, 2019

fix: values() was incorrectly passing a MapIterator to the JavaArray constructor instead of an array causing a runtime error.

feature: entrySet() now returns a JavaArray of key,value JavaMaps

JavaMap was incorrectly passing a Map Iterator to the JavaArray class instead of an array.
@lightsofapollo
Copy link
Contributor

awesome

@lightsofapollo lightsofapollo merged commit ca6ff22 into little-bear-labs:master Jan 25, 2019
@thisjeremiah
Copy link

Does this mean the $util.map util functions also need to be updated? I'm getting some strange results using $util.map.copyAndRemoveAllKeys.

@cbaron
Copy link
Contributor

cbaron commented Jan 31, 2019

@thisjeremiah -- most likely, feel free to put up a PR ( and tests if you can ) !

@marklawlor
Copy link
Contributor Author

@thisjeremiah @cbaron After this PR I've run into a couple of issues. This PR has exposed a deeper problem with the JavaArray class. The problem is that JavaArray extends Array but does not implement the same constructor as the native Array class. This means that native Array functions (such as map) behave incorrectly with the JavaArray class. For reference, native Array functions will call the constructor method with a number for the array's length, and then it populates that indexes. This is causing an forEach is undefined error.

@cbaron
Copy link
Contributor

cbaron commented Jan 31, 2019

@marklawlor -- thanks for digging in a bit more and providing us with information. Currently, I do not have bandwidth to iterate on a solution, but again, will gladly accept PRs. While I will not require tests ( as we did not originally write any ), I think they would be helpful in the long run.

@marklawlor
Copy link
Contributor Author

I created an issue to track the topic #76

I haven't used the $util.map functions, I'm just assuming they use the native map function under the hood. So this might be a completely different issue

@thisjeremiah Can you confirm that the code is failing on the forEach statement? If you are not getting any logs, you can add a console statement to line 35 of node_modules/@conduitvc/appsync-emulator-serverless/lambdaRunner.js

@marklawlor
Copy link
Contributor Author

Now that I'm glancing at the code, I think @thisjeremiah has a different issue.

copyAndRetainAllKeys and copyAndRemoveAllKeys won't work because map is a Map and not an object

const map = new Map([[ 1, 'one' ],[ 2, 'two' ]])
Object.entries(map) === [] //  true
{ ...map } === {} //  true

@marklawlor
Copy link
Contributor Author

PR is up #77.

@cbaron I'm at work so I can't test it right now. If someone from @conduitvc has time to test that would be awesome, otherwise I'll get to it over the weekend.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants