-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: Fix timezone Date parsing for Hermes JS engine #2843
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
base: dev
Are you sure you want to change the base?
fix: Fix timezone Date parsing for Hermes JS engine #2843
Conversation
Wow I've been needing this! |
Can we get this merged before another dev loses their mind trying to debug this? 🚀🔥 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 !!!
Similar to #2227 but I think simpler because it just uses dayjs itself to do the parsing here. Either works though! |
Also this Issue: |
🥇 |
Holy smokes he solved it ! |
oh boy I came to look for this exact issue, it's been driving me nuts for a day here. Hopefully this gets merged soon, thank you |
looks like this should fix #1377. |
@iamkun can you please take a look and merge it in? if it fixes a tz support for React-Native with Hermes — it would be a huge win! |
This is great!!!!! @milesingrams 🚀 Coming here from @dlebedynskyi mention of this fix in #1377 Maybe this isn’t the right place to ask for help about this issue, but I thought I’d share my experience in case anyone else is trying the same thing. I tried using this branch as the reference for mkdir dayjs-tz-patch-milesingrams
cd dayjs-tz-patch-milesingrams
git clone --single-branch --branch fix-timezone-date-parsing-for-hermes-js-engine https://github.com/milesingrams/dayjs.git
cd dayjs
npm install
export NODE_OPTIONS=--openssl-legacy-provider
npm run build
npm pack This created the file npm install /absolute/path/to/dayjs-tz-patch-milesingrams/dayjs/dayjs-0.0.0-development.tgz Despite this, the timezone behavior was still broken when running with Hermes — it was as if the fix wasn’t applied. I was hoping that anyone here can confirm — if this is the correct way to apply and test a patch in a project? Just want to make sure I didn’t miss something! |
This update does improve the timezone plugin using hermes although the test suite still shows some problems. For reference I had to build a custom version of hermes to include support for
Then I used the the
|
@konradjg Been following this thread closely and been looking at a lot of other threads and I think that nothing will get merged soon due to the fact that this is not a dayjs issue at all. It is mostly hermes issue and the main maintainer there have considered it super low priority facebook/hermes#1519 (comment) - For now I just went ahead with the ployfills suggested in #1377 (comment) and I think there is nothing wrong with that. At least for me it is doing the job it needs to. I may try in the following days to switch the engine to JSC or v8 as there are speculations online that it may make the app faster due to date manipulation e.g. facebook/hermes#930 or facebook/hermes#495 or hermes_very_slow_compared_to_jsc_with_big_data |
When using the Hermes engine in the newer versions of React Native the timezone plugin doesn't work because there are a few places where the
Date
constructor is used to parse aen
locale formatted date string and Hermes doesn't support that format of parsing (i.e.new Date('8/20/1999, 5:00:15 AM')
). This issue results in the timezone plugin always returning GMT and UTC times when used in newer Expo and React Native apps. In order to work around this we can use dayjs itself to parse these dates instead of the native Date constructor. This PR allows dayjs to work in Expo and React Native with the timezone plugin.