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 for File.lastModified on IE10 and IE11 #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rspi
Copy link

@rspi rspi commented Oct 1, 2019

IE11 does not have lastModified so we fall back on lastModifiedDate.

Note that lastModifiedDate returns a date-object and not an integer, but the time library
handles this correctly. Alternatively one could do file.lastModifiedDate.getTime() to extract
time as in integer.

@evancz
Copy link
Member

evancz commented Nov 13, 2019

Initial review suggests that the following code would work in a broader range of scenarios:

function _File_lastModified(file)
{
	// Check the deprecated lastModifiedDate field for IE10 and IE11.
	//
	// If that fails too, give the current time. This is the fallback behavior
	// for lastModified in the specification: https://www.w3.org/TR/FileAPI/
	//
	return __Time_millisToPosix(
		'lastModified' in file
			? file.lastModified
			: 'lastModifiedDate' in file
				? file.lastModifiedDate.getTime()
				: (file.lastModified = Date.now())
	);
}

This way (1) it is not left up to trust that the date is converted to a POSIX time and (2) you cannot get a crash if lastModifiedDate does not exist.

We need to do more testing before proceeding with this here, particularly around the behavior of Blob files on different OSes and browsers from your other issue.

@rspi
Copy link
Author

rspi commented May 15, 2020

Any updates on this?
It would be good to have this merged at least as an incremental improvement by making it work in IE11.
I can change the PR to your initial suggestion.

Been running this code and my other PR in prod since October without any reported issues :)

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.

2 participants