Skip to content

makeCursor may return incorrect cursor type #297

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

Closed
wants to merge 1 commit into from

Conversation

jcranendonk
Copy link

Certain code paths call makeCursor with four arguments, with 'value' undefined. Since arguments.length is still four in this case, the value is not retrieved from rootData, and an incorrect cursor type may be generated.

Fixes #318

(As a side effect, this change may also make makeCursor eligible for VM optimizations.)

Certain code paths call makeCursor with four arguments, with 'value'
undefined. Since arguments.length is still four in this case, the
value is not retrieved from rootData, and an incorrect cursor type can
be generated.
var data = Immutable.fromJS({ a: [1, 2, 3] });
var cursor = Cursor.from(data);
var deepCursor = <any>cursor.cursor('a');
expect(deepCursor.findIndex).toBeDefined();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the best way to test this. I'm open to suggestions.

@tcoopman
Copy link
Contributor

Any updates on this?

@leebyron
Copy link
Collaborator

Thanks for adding this. I'm a bit behind on reviewing PRs, but I promise to dig into it soon.

@jeffbski
Copy link
Contributor

jeffbski commented Mar 8, 2015

+1

@jeffbski
Copy link
Contributor

jeffbski commented Mar 9, 2015

I created an alternate PR for this #381 which continues to use argument.length as the condition since I think it might be valid for value to be undefined in some circumstances. subCursor() simply needs to do an argument.length check as well and call makeCursor appropriately. I reused @jeroencranendonk-wf 's test.

@leebyron
Copy link
Collaborator

Closing in favor of @jeffbski's fix.

@leebyron leebyron closed this Mar 27, 2015
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.

cursor.cursor('path') to immutable.List returns wrong cursor type
5 participants