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

Remove Luxon validation, so the script can deliver proper results in any supported Node.js version. #271

Open
chekrd opened this issue Oct 19, 2022 · 2 comments

Comments

@chekrd
Copy link

chekrd commented Oct 19, 2022

Hi @vvo, thanks for this package, it helped us a lot.

This is more of a discussion than an issue. It is a follow up for already fixed issue #267.

I was part of the investigation in cooperation with NBaruK.

I would like to know the reason behind the following code (link to the module):

const tz = DateTime.fromObject(
  {},
  {
    zone: timeZoneName,
  },
);

if (tz.isValid !== true) {
  console.error(
    "Time zone data not accurate, please investigate",
    tz.invalidReason,
    timeZoneName,
  );
  continue;
}

DateTime.fromObject comes from Luxon package. Then, inside Luxon, the validation of the input is done using Intl API (link to the module within Luxon).

This is the reason why platform version (Node.js) affects the ouput. And I can't understand the real meaning of the timezone validation in generate.js. Timezones input is from IANA - I would say we can trust it as it is a source for Intl used by Node.js, browsers, etc.). So you validate the timezone against the same database, but if the current installed platform (nodejs) is not up to date with the IANA DB, it fails. It does not mean the input is wrong.

I would be glad if you can elaborate on the code. Is it necessary? Can you remove this validation from your package?

@chekrd chekrd changed the title Remove Luxon validation, so the script can deliver proper results in any supported NodeJs version. Remove Luxon validation, so the script can deliver proper results in any supported Node.js version. Oct 19, 2022
@vvo
Copy link
Owner

vvo commented Oct 20, 2022

This was 2 years ago, and I do not remember why this was added. Probably because I use these time zone names on the backend/frontend to generate dates myself using Luxon in a project. So if the timezone names doesn't pass Luxon's validation it would fail for me too.

Is this currently causing any trouble? Thanks!

@chekrd
Copy link
Author

chekrd commented Oct 20, 2022

As long as you are using IANA (that's what tzdb is about), Luxon validation in tzdb is useless as I described in the first post. The only thing Luxon validation can do is remove valid input timezones based on the Node.js version, thus silently breaking a final app. I guess you didn't know you may stopped supporting Kiev timezone in your projects out of a sudden.

Is this currently causing any trouble?

Currently no. But the source of the issue is not fixed, so we can expect regression anytime.

That's why I think Luxon validation should be removed.

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

No branches or pull requests

2 participants