thejoshwolfe / yauzl

yet another unzip library for node
MIT License
678 stars 77 forks source link

Fix time zone offset being applied to dates in dosDateTimeToDate. #138

Closed BillyONeal closed 3 months ago

thejoshwolfe commented 3 months ago

The existing behavior is intended and documented. Feel free to make a case for changing the intended behavior.

BillyONeal commented 3 months ago

The existing behavior is intended and documented. Feel free to make a case for changing the intended behavior.

I believe it causes archives to be reported with incorrect times; notably, the time zone when an archive was minted is unrelated to when and where an archive is unpacked.

However, given that it is documented, I agree at this point it would be a breaking change to do anything. Thanks!

thejoshwolfe commented 3 months ago

It sounds like you've got a usecase where the caller of the function knows what specific timezone should be used, in your case UTC. It would be cool to add an optional parameter to dosDateTimeToDate that affects the timezone behavior.

After doing some research, however, it seems like there's just no way to specify a timezone on a Date object, except for either local or UTC. Furthermore, web experts are furious enough about how deficient the Date api is in JavaScript that an overhaul api is working its way into the standard called Temporal. It's currently at stage 3. https://github.com/tc39/proposal-temporal

I'm inclined to hold off on adding timezone support to yauzl until we can do it properly with the Temporal api, but maybe a compromise in the meantime could be made if you've got a case for UTC. Let's see how hard is it to correct the timezone after calling yauzl's API...

var timestampInterpretedAsLocal = entry.getLastModDate();
var timestampInterpretedAsUTCInstead = new Date(
    timestampInterpretedAsLocal.getTime() -
    timestampInterpretedAsLocal.getTimezoneOffset() * 60 * 1000
);

Not great, but not terrible. The logic here is that the local timezone offset is applied to the timestamp, but we want to take it out, so that's why it's a subtraction.

I'll add this workaround to the docs.

BillyONeal commented 3 months ago

Our use case was 'extract zips downloaded from the internet' and for better or worse it seems like every example we tried was in UTC in the file. As a result, when we printed the results from yauzl on the terminal, it disagreed with 7zip et al.

It sounds like you've got a usecase where the caller of the function knows what specific timezone should be used, in your case UTC. It would be cool to add an optional parameter to dosDateTimeToDate that affects the timezone behavior.

I think this is backwards: what we wanted was local time, but times in the zip were UTC.

Unfortunately we switched to 7zip for this so I don't have a yauzl example set up right now to try...

BillyONeal commented 3 months ago

I don't know they seem to agree now though :shrug:. I think just closing this was the right call

image