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

Allow to set a custom TimeZoneInfo on the ScriptEngine, and avoid DateTime.MinValue as invalid date #69

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

kpreisser
Copy link
Collaborator

@kpreisser kpreisser commented Oct 30, 2016

Hi,

it would be cool if Jurassic's ScriptEngine allows to set a custom TimeZoneInfo used to convert between UTC and local time. This is useful for example if you run user-defined scripts on a server, but each script should run with the user's timezone (Jint also allows to set the engine's timezone).

While setting the current CultureInfo (for number-formatting etc.) can be done on the current thread (so this works without modifying the ScriptEngine), there is not thread-specific timezone setting, so this would need to be done on the ScriptEngine level.

This PR creates a new property ScriptEngine.LocalTimeZone where a custom timezone can be set, which is then used by the DateInstance. The default value is TimeZoneInfo.Local.

The DateInstance's internal DateTime value is now also always UTC, whereas previously it sometimes was local time and sometimes was UTC.

Update: Additionally, this PR also avoids using DateTime.MinValue to represent an invalid date. Generally, such uses of data types should be avoided because this can lead to problems where such a value is actually expected to be used.

For example, we use Jurassic in a product where users can call .NET APIs, and one of them expects a DateTime range. In order to specify the full range, one would need to specify DateTime.MinValue and DateTime.MaxValue (in JS: new Date(-62135596800000) and new Date(253402300799999) (although the latter is actually a bit lower because it doesn't include the nanoSeconds). Therefore, I think Jurassic should allow to use DateTime.MinValue as valid date.

Thanks!

@kpreisser kpreisser requested a review from paulbartrum January 4, 2017 14:01
…hat is a valid .NET DateTime and can lead to errors when calling .NET APIs.
… valid when the year is set.

This is also the behavior in other JS engines.
@kpreisser kpreisser changed the title Allow to set a custom TimeZoneInfo on the ScriptEngine Allow to set a custom TimeZoneInfo on the ScriptEngine, and avoid DateTime.MinValue as invalid date Jan 6, 2019
@kpreisser
Copy link
Collaborator Author

Hi @paulbartrum,

I rebased the PR on the current master branch.

Additionally, I removed the usage of DateTime.MinValue to represent an invalid date, because this can lead to problems when such a value is actually expected. For example, in our product we use Jurassic where users can call .NET APIs, and when I tried to provide the minimum DateTime value (new Date(-62135596800000)) for a filter, I got an Invalid date which was unexpected.

Instead, I'm using null to represent an invalid date now.

Can you review the PR?
Thank you!

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.

1 participant