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

timezone is ignored with parseJSON #2149

Closed
DaSchTour opened this issue Jan 9, 2021 · 8 comments · Fixed by #2200
Closed

timezone is ignored with parseJSON #2149

DaSchTour opened this issue Jan 9, 2021 · 8 comments · Fixed by #2200

Comments

@DaSchTour
Copy link

DaSchTour commented Jan 9, 2021

I made this small test https://runkit.com/daschtour/parsejson-timezone and found that when using parseJSON on a date string with timezone it get's lost in the conversion. So for my case using one date converting it to JSON and converting it back to a date adds one hour, although the timezone offset is added back correctly.

@tan75
Copy link
Contributor

tan75 commented Feb 1, 2021

@DaSchTour
what is your timezone that was used for this test?

@DaSchTour
Copy link
Author

@tan75 GMT+0100

@tan75 tan75 added the 🐛 Bug label Feb 2, 2021
@tan75
Copy link
Contributor

tan75 commented Feb 2, 2021

@DaSchTour
Yes, it looks like its a bug thats applies to your timezone.
Thanks for reporting this!

@alarangeiras
Copy link

alarangeiras commented Feb 11, 2021

I have a suggestion. Instead of create a Date.UTC with each part of the string we can use the default API to parse it. Like below:

return new Date(argument);

Below follow the whole parseJSON function.

export default function parseJSON(argument) {
  requiredArgs(1, arguments)

  if (typeof argument === 'string') {
    var parts = argument.match(
      /(\d{4})-(\d{2})-(\d{2})[T ](\d{2}):(\d{2}):(\d{2})(?:\.(\d{0,7}))?(?:Z|\+00:?00)?/
    )
    if (parts) {
      return new Date(
        Date.UTC(
          +parts[1],
          parts[2] - 1,
          +parts[3],
          +parts[4],
          +parts[5],
          +parts[6],
          +((parts[7] || '0') + '00').substring(0, 3)
        )
      )
    }
    return new Date(NaN)
  }
  return toDate(argument)
}

@alarangeiras
Copy link

alarangeiras commented Feb 11, 2021

@tan75 I've made some tests locally and the proposal converts correctly. If you agree, I can do the change.

@tan75 tan75 self-assigned this Feb 11, 2021
@tan75
Copy link
Contributor

tan75 commented Feb 12, 2021

@tan75 I've made some tests locally and the proposal converts correctly. If you agree, I can do the change.

hi @alarangeiras Thanks a lot for the suggestion. Let me investigate this issue a bit.

@alarangeiras
Copy link

@tan75 I've made some tests locally and the proposal converts correctly. If you agree, I can do the change.

hi @alarangeiras Thanks a lot for the suggestion. Let me investigate this issue a bit.

Hi @tan75, you're welcome... let me know if i can help you with the fix.

@tan75
Copy link
Contributor

tan75 commented Feb 15, 2021

@tan75 I've made some tests locally and the proposal converts correctly. If you agree, I can do the change.

hi @alarangeiras Thanks a lot for the suggestion. Let me investigate this issue a bit.

Hi @tan75, you're welcome... let me know if i can help you with the fix.

Hi Allan,

after doing some digging, it was discovered that the issue was in the Regex that was matching the dates - it supported only zero offsets. I fixed it in one of my PRs and hopefully it will be approved and released soon.
Thanks a lot for your input - we really appreciate it.
Please feel free to contribute and please reach out if you have any questions or suggestions.

@tan75 tan75 linked a pull request Feb 15, 2021 that will close this issue
kossnocorp pushed a commit that referenced this issue Feb 22, 2021
Added support of positive and negative offsets in `parseJSON` (#2149).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants