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

fix(filesystem): Convert stat ctime/mtime timestamp to milliseconds #321

Merged
merged 6 commits into from
Mar 24, 2021

Conversation

theproducer
Copy link
Contributor

This PR makes Filesystem.stat return ctime and mtime in milliseconds iOS, bringing it in line with Android.

closes: #313

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

iOS is returning decimals, while Android and web aren't, so I think iOS shouldn't return the decimals neither.

The types should be updated to specify that it returns the value in milliseconds.

Android returns null for ctime, seems like it's possible to get the value on Android 26+, so we should probably return the value when available.

Should be documented on the upgrade guide since it can be considered a breaking change for iOS.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Looks like the creation time code is not working, it returns the last modified value too.
So not sure if it's best to remove it again.

I've noticed that lastAccessTime seems to be the creation date, so maybe we can return the min between that and creationTime (just in case it's an android bug and they fix it in the future). That or remove the whole ctime return.
If we keep the ctime return, instead of 26 it's better to use Build.VERSION_CODES.O, and as there is already a data.put("ctime", null);, put that in the else of the android version check.

Also the README need to be re generated since the types changed.

@theproducer
Copy link
Contributor Author

Thanks - I'll open a PR in the docs for updates to the upgrade guide.

@jcesarmobile
Copy link
Member

Not sure if you saw my last comment since I requested changes and you requested review again without making any changes

@theproducer
Copy link
Contributor Author

Sorry, just saw them!

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

looks good

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.

capacitor-filesystem: stat return inconsistent timestamps between platforms
2 participants