Skip to content
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

[KBFS-3561] Fix downloads on Android #14585

Merged
merged 4 commits into from Nov 5, 2018

Conversation

jzila
Copy link
Contributor

@jzila jzila commented Nov 5, 2018

This might warrant a hotfix.

@jzila jzila requested review from taruti, songgao and a team November 5, 2018 20:09
@taruti
Copy link
Contributor

taruti commented Nov 5, 2018

LGTM, but cannot test at the moment.

@songgao
Copy link
Contributor

songgao commented Nov 5, 2018

Would mime.mimeType not work?

@jzila
Copy link
Contributor Author

jzila commented Nov 5, 2018

Yes it works, I just like leaving explicit evidence that this is an immutable object in the code.

@songgao
Copy link
Contributor

songgao commented Nov 5, 2018

But it's not type-checked anymore right? For example, you could do mime.get('MimeType') and it'd pass flow, I think.

@jzila
Copy link
Contributor Author

jzila commented Nov 5, 2018

It appears to me that the type checking here is completely broken anyway, since Flow didn't catch this error in the first place.

@jzila
Copy link
Contributor Author

jzila commented Nov 5, 2018

Does type checking work for Immutable records e.g. with .mimeType?

@chrisnojima
Copy link
Contributor

i agree, avoid get unless its a dynamic sub thing

@songgao
Copy link
Contributor

songgao commented Nov 5, 2018

It appears to me that the type checking here is completely broken anyway, since Flow didn't catch this error in the first place.

That can't be an argument for giving up type checking there right? I think the reason why flow didn't catch it was probably because of the less typed generator function. But perhaps something like this would work:

    const mimeType: Types.MimeType = yield Saga.call(_loadMimeType, path)

Also I'm guessing the type checking must be working in viewTypeFromMimeType.

shared/fs/index.stories.js Outdated Show resolved Hide resolved
@chrisnojima
Copy link
Contributor

they flow type the same. .get isn't required for records and we shouldn't use it

Copy link
Contributor

@songgao songgao left a comment

Choose a reason for hiding this comment

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

LGTM; Thanks for making the changes!

shared/actions/fs/index.js Outdated Show resolved Hide resolved
@jzila jzila merged commit 672d571 into master Nov 5, 2018
@jzila jzila deleted the jzila/KBFS-3561-fix-bad-android-downloads branch November 5, 2018 21:43
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.

None yet

4 participants