Stop type-checking for 'object' to allow functions as well

[cooperka]

Fixes https://github.com/vacuumlabs/babel-plugin-extensible-destructuring/issues/19.

No need to type check because we'd just re-throw anyway, and default error is pretty clear -- trying to invoke 'foo' in 'bar' results in Cannot use 'in' operator to search for 'foo' in bar (at least with Chrome dev tools).

Let me know if you'd prefer to go a different route, e.g. check typeof for both 'object' and 'function'. Simply checking instanceof Object would be even better, but unfortunately Immutable tends to create its own Object that's different from the global one so that won't work.

See discussion below for more info.

[cooperka]

@hleumas what do you think of this?

[hleumas]

@tomaskulich what is your opinion?

[tomaskulich]

Hi @cooperka thanks for the nice PR! What you're saying makes sense, however, I'd prefer the second approach you're suggesting - checking typeof for both 'object' and 'function'. The reason for this is that I'm not sure about the error message which using in gives - if you compare this to the original:

`cannot resolve property ${k} in object of type ${typeof o} (${safeString(o)})`

it seems that there is much more useful debug information here, right?

[cooperka]

@tomaskulich fair point, I will keep the debug message. However, now that I think about it more, there are actually some properties that can be destructured from other types of objects as well. For example:

const { length } = ''; length === 0
false['toLocaleString']() === 'false'

It would arguably be silly or pointless to try to destructure things like this, but it does work in plain JS and will break with the current implementation of extensible-destructuring.

It turns out using brackets to access any arbitrary property of any variable always returns undefined if there's no such property, and it doesn't produce an error. Even [][1] is simply undefined. Please correct me if you know of an edge case, but I have updated the code to use this technique. It should be both more performant and much more flexible!

Update: null['key'] or undefined['key'] will throw an error. I will add logic for this.

Now in this case there's no reason to throw, because there are no errors generated, and we can simply return the default value if o[k] is undefined. What do you think of this?

---

Further, we no longer need to throw if typeof k !== 'string', and we can allow things like using Symbols for keys. I didn't make this change yet because I don't want to make this PR too significant, but I'm happy to take it farther if you approve.

[cooperka]

I think there are also tests that I haven't run, I'll update those soon as well (assuming you're ok with moving forward with these changes).

[cooperka]

@tomaskulich ping :) curious what your thoughts are about the changes.

[tomaskulich]

Hi @cooperka, I like very much how it is right now - I believe that this approach should generally work really well.

Before merging, could you please check, whether the tests are running (and maybe on some simple example)?

Finally, thank you very much for your work! Also, would you be interesting in contributing to the project more? I believe there are few issues, which could be addressed.

[cooperka]

I verified locally with some example code, but I'll add tests and make sure it's all working before we merge (I'll try to get to it tonight).

I can't guarantee I'll have too much extra time for contribution, but think this is an awesome project and I use Immutable all the time, so I'm sure the issues will bite me eventually anyway ;) I'd happily try to address some of the issues as a collaborator/member/whatever GitHub calls it now.

[cooperka]

@tomaskulich any advice on running tests? After installing and building, I try to run npm test but I get the error:

> node runtest

Unhandled rejection ReferenceError: Unknown plugin "./lib" specified in "base" at 0, attempted to resolve relative to "/Users/me/forks/babel-plugin-extensible-destructuring/test"
    at /Users/me/forks/babel-plugin-extensible-destructuring/node_modules/babel-core/lib/transformation/file/options/option-manager.js:180:17
    at Array.map (native)
    at Function.normalisePlugins (/Users/me/forks/babel-plugin-extensible-destructuring/node_modules/babel-core/lib/transformation/file/options/option-manager.js:158:20)
    ...

I should also make a PR for adding local dev instructions to the README ;)

[tomaskulich]

hm, I tried it and it worked. I've deleted node_modules and reinstalled and now I end up with the same error as you.. Probably they changed what is the base path for resolving the babel plugins.. I'll think how to fix this.

[tomaskulich]

I've fixed it - check out the newest master

[cooperka]

@tomaskulich somehow CircleCI is "passing", but it gets the same error as I do now running tests:

Unhandled rejection Error: Cannot find module '../../runtime'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)

https://circleci.com/gh/vacuumlabs/babel-plugin-extensible-destructuring/23

[cooperka]

(fixed)

[cooperka]

@tomaskulich I added a few more commits -- the new behavior is now tested, tests work locally, and all error cases are handled :) Ready to merge when you are, or happy to make further tweaks.

[tomaskulich]

Merged; thanks for the nice contribution! I've also fixed the dependencies in the main package.json to use the 'file:path' syntax (though I'm not sure, whether symlinks wouldn't be better for development..)

[cooperka]

Ah, you're probably right that symlinks would be better, I'm just used to using the file syntax because React Native (or, more specifically I think it's Watchman) doesn't yet support symlinks :(

[cooperka]

Oh @tomaskulich would you also mind inviting me as a collaborator? I'd be happy to try to resolve some other issues, especially as related to React Native.

[hleumas]

@cooperka welcome:-) I have added you as a collaborator.

[hleumas]

Thanks for the interest and all the help!

[cooperka]

Thank you @hleumas :) I'll do what I can to help.

[tomaskulich]

@cooperka yeah, I know watching over symlinked folder is pain. The way it is now is OK, but not so great for a rapid development :)

Thanks for the contribution, let me know if you're about working on some issues.