-
Notifications
You must be signed in to change notification settings - Fork 53
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
Date handling when simplifying date strings without a timezone seems incorrect #14
Comments
Actually after giving this a bit more thought... my suggestion would be to remove the simplify option completely and focus only on parsing. Let a more complete library like |
Hi, I second that: thanks for this awesome EXIF parser! I think I have similar issues with Date/Time Original: 2017:01:04 11:29:12 # matches the file name When I use exifparser.create(fileContents).parse().tags.DateTimeOriginal)
// returns 1483529352
new Date(1483529352 * 1000)
// Wed Jan 04 2017 22:29:12 GMT+1100 (AEDT) So it looks like it assumed GMT and added 11 hours for AEDT which is my local timezone. I started noticing because I just added the "create albums by date" functionality to thumbsup (which uses |
Wow, this is an old one. Sorry I missed this for so long. Great catch about the default time zone. Also the parsing for explicit time zones is quite incomplete. Briefly looking at the spec; optional colons between hours and minutes of the offset are not handled and the I would not remove simplifying EXIF values completely, as Splitting up in two libraries would be too much overhead as the release process is already somewhat cumbersome. I would be very happy with a PR for fixing the default time zone to local time, or a setting to just disable simplifying dates :) |
Could use this for the parsing: https://www.npmjs.com/package/iso8601-convert |
Just coming to this package for the first time, and ran into this issue. I assume it remains unresolved? A few notes from the EXIF 2.2 spec:
I'm not sure it would be great to return the local time, when this library can be used server side and the client's notion of timezone may differ. I can see a couple of options:
Personally I think Option 2 is a bit better. Thoughts? |
Fix bwindels#14 by not converting to local or UTC date, instead slightly modifying DateTime strings into easy-to-parse ISO 8601 format. When a DateTime string follows the EXIF spec, we map some colons to dashes to conform to ISO 8601. The other supported format was ISO 8601, so we don't do anything in this case.
Fix bwindels#14 by not converting to local or UTC date, instead slightly modifying DateTime strings into easy-to-parse ISO 8601 format. When a DateTime string follows the EXIF spec, we map some colons to dashes to conform to ISO 8601. The other supported format was ISO 8601, so we don't do anything in this case.
Yeah, parsing dates with a timezone needs to be improved. I'd rather do it with a very light dependency as mentioned before, or none at all. moment.js is 16kb minified, too big to justify for this problem IMO. Apart from that, one thing mentioned in earlier comments is that some tools assume that a date without a timezone is assumed to be in the local timezone. An option to set the default timezone could make sense. I'll do it when I get to it, unless anyone beats to me making a PR :) |
The time zone is wrong |
Care to elaborate? Can you share the date string in the file you see the issue with? |
like #14 (comment) I test Relevant links found by the way I test |
First off... thanks for the library. Of the ones that I have seen it is by far the most complete.
However, it seems like your logic when simplifying a date string may be wrong. As far as I can tell you are treating a date string without the an explicit timezone as UTC time when according to ISO standards it should be treated as local time 1.
If you agree that this is a problem I can open a pull request with a fix...
The text was updated successfully, but these errors were encountered: