search

AckerApple / angular-file

Angular components for user file select, drop, and more
http://ackerapple.github.io/angular-file/
MIT License
128 stars 40 forks source link

Null check missing on variable 'transfer' #52

Closed tarikbuza closed 5 years ago

tarikbuza commented 5 years ago

https://github.com/AckerApple/angular-file/blob/79e3a71f14e6c8e9db579bfd716f19da13c54bda/src/file-upload/ngf.directive.ts#L220-L223

When event.dataTransfer or event.originalEvent.dataTransfer are undefined or null, function eventToTransfer returns null. Then, there is no check if variable transfer is null or not in function eventToFiles. This leads to 'Cannot read property 'files' of null' error.

https://github.com/AckerApple/angular-file/blob/0462b824eedc1dd5f1e43b3ff5b12b7d5ecdab40/src/file-upload/ngf.directive.ts#L252-L257

AckerApple commented 5 years ago

I understand the scope of what you are presenting. I've not come across a situation in which event.originalEvent was undefined, that I know of.

Upfront, maybe the answer is to just check that eventToTransfer returned something. However, what's causing event.originalEvent to be undefined I think is the greater concern... Perhaps the "fix" is to use event when event.originalEvent is undefined?

I am unsure how to proceed and no time available to devote. Just lost my job. A pull request will be needed if you need this fixed

tarikbuza commented 5 years ago

Problem occurs when I use file picker inside the container with ngfDrop directive. Then comes this error. There should be a check for null, since there is a case in which it is returned from eventToTransfer function. In case transfer is null, simply returning []; solves the problem for me. I would really need this pull request.

AckerApple commented 5 years ago

it sounds like you know what's up. Do you know how to fork and then pull request? If so, please do and I will test and publish

AckerApple commented 5 years ago

Just in case, here is a great video on how easy it is to fork and then make a pull request: https://www.youtube.com/watch?v=dSl_qnWO104

tarikbuza commented 5 years ago

Thank you for the link, I have created the pull request.

AckerApple commented 5 years ago

You almost did. Some how the pull request is within your fork. BUT it should be a pull request against my package. Like it’s 99% right but you selected one wrong option at the end I think.

Your pull request is appearing incorrectly here: https://github.com/tarikbuza/angular-file/pulls

When it actually needs to be seen here: https://github.com/AckerApple/angular-file/pulls

Where your pull request is right now, only you can work with it. When you correctly issue a pull request to my package, I will get merge controls

tarikbuza commented 5 years ago

Ah I see it now, thanks. Must have done something wrong. Is it okay now?

AckerApple commented 5 years ago

Merged pull request. Issue should be resolved

tarikbuza commented 5 years ago

Thank you very much once again :grinning: