This repository has been archived by the owner. It is now read-only.

Fix crash when exporting with destructuring and sparse array #170

Merged
merged 2 commits into from Oct 12, 2016

Conversation

Projects
None yet
2 participants
@jfmengels
Copy link
Contributor

jfmengels commented Oct 12, 2016

Hi there!

The tests for eslint-plugin-import are currently failing, with the Cannot read property 'type' of null error.

I tracked down the error to Babylon and the new checks to whether a duplicate export exists, and have fixed it in this PR.

I have created a minimal viable example, which is available in my first commit, to show that the tests fail unexpectedly with the error message above. It looks like this

export const { foo: [ ,, qux7 ] } = bar;

(Actually, export const [,, a] = {} would have sufficed, but I just noticed it, and have already updated the expected.json file in the tests, which took me a while 馃槄... let me know if you prefer I use this form instead, or add both cases in the tests)

The problem is that the code here loops through the elements inside the destructuring expression, but forgets to filter out the null ones, that you get when you have a sparse array like [,,a], before directly accessing the type property.

I added a null check around the recursive call, and that fixes the problem.

Let me know if you need more info, more work or anything else :)

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 12, 2016

Nope that looks good/makes sense! Thanks a lot!!

@hzoo hzoo added the Tag: Bug Fix label Oct 12, 2016

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 12, 2016

and have already updated the expected.json file in the tests, which took me a while

@jfmengels If you create an actual.js without an expected it should automatically make the file - hopefully you didn't mean you did it manually 馃槄 , but if so next time!

Will merge + release after travis is done

@hzoo hzoo merged commit e14f93d into babel:master Oct 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jfmengels jfmengels deleted the jfmengels:export-destructuring-crash branch Oct 12, 2016

@jfmengels

This comment has been minimized.

Copy link
Contributor Author

jfmengels commented Oct 12, 2016

hopefully you didn't mean you did it manually 馃槄 , but if so next time!

Haha, I did... 馃槄

I didn't know how tests were created, so I just updated an existing test, which already had the file. Deleting it was not the most intuitive thing to do 馃槃.

Honestly I just thought you were working like a madman to have proper expected.json files. This makes much more sense 馃槃. (Though, if you don't write it at least in part manually, I'm not sure what exacly you're testing, but that's another topic). I myself cheated a bit by using astexplorer.net, and then manually fixing every error the tests showed me. I will definitely remember it next time.

Will merge + release after travis is done

Awesome! Thanks for being so reactive!

kristofdegrave pushed a commit to kristofdegrave/babylon that referenced this pull request Oct 27, 2016

Fix crash when exporting with destructuring and sparse array (babel#170)
* Create reproducible crash when exporting with destructuring and sparse array

* Fix crash when exporting with destructuring and sparse array
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.