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

Problem with addLinks method #301

Open
stestone opened this issue Feb 19, 2016 · 6 comments
Open

Problem with addLinks method #301

stestone opened this issue Feb 19, 2016 · 6 comments
Labels

Comments

@stestone
Copy link

In the addLinks method around line 426 there is the following code:

links[normal0] = normal1;
names[normal0] = alias[0];

links[normal1] = normal0;
names[normal1] = alias[1];

Does this technically add a link that doesn't need to be there since links[normal0] is really the one they are all linking to? It might be by design and perhaps I'm not understanding how it works.

The reason it was causing me problems is I was wanting to get a shortened list of moment.tz.names() based on the links but it kept coming out twice as long as it needed to be when I used moment.tz._links to filter it. Does that make sense?

@timrwood
Copy link
Member

What do you need the filtered list for? Perhaps we can design a better api that can do this filtering for you.

@stestone
Copy link
Author

I'm using it to populate a select form element but when you populate it based on the names() variable, it just becomes to much information, so I was looking into the links as a way to cull this list down so that the user is presented with less options. Looking at the links that are given in the data it should take the list down to about 80, but what I was finding was it actually comes out to twice that much and the reason is I think because of what I mentioned above. It's creating links that don't need to be there with the following line:

links[normal0] = normal1;

If you watch what the loop is doing this value would be overwritten with each link that shares the same name anyways. So you end up with an array of 160 instead of 80. I commented out that line and it fixed the issue for me.

@timrwood
Copy link
Member

We maintain a list of approximate populations for many timezones that could also be used to filter the list.

We could also consider adding a moment.tz.guesses() api to expose a list of potential matches to the users' timezone.

I agree that we need to provide some sort of filtering, but I don't think the internal _links map should be used for this.

@gilmoreorless
Copy link
Member

This problem ended up causing a bug in a webpack plugin that makes heavy use of filterLinkPack. As described in gilmoreorless/moment-timezone-data-webpack-plugin#6:

For example, the latest dataset in moment-timezone includes a link from Europe/Prague to Europe/Bratislava: Europe/Prague|Europe/Bratislava. Running the plugin with {startYear: 2000} would keep that link, even though Europe/Prague becomes a link itself (Europe/Paris|Europe/Prague). This would result in a runtime error in moment-timezone: Moment Timezone has no data for Europe/Prague.

When using filterLinkPack() or filterYears() with a start year of 2000 (as an example), then loading the subsequent data, the following process happens:

  1. The zone Europe/Paris is parsed and added to moment.tz._zones (as europe_paris).
  2. The link Europe/Paris|Europe/Prague is parsed and added to moment.tz._links:
    • links['europe_paris'] = 'europe_prague'
    • links['europe_prague'] = 'europe_paris'
  3. The link Europe/Prague|Europe/Bratislava is parsed and added to moment.tz._links:
    • links['europe_prague'] = 'europe_bratislava' (this rewrites the Prague link to be incorrect)
    • links['europe_bratislava'] = 'europe_prague'

If any code then tries to use Europe/Prague, it fails with the error "Moment Timezone has no data for Europe/Prague." This is because the getZone method does the following:

  1. Check if europe_prague is a zone (false).
  2. Check if europe_prague is a link (true).
    1. Look up what europe_prague links to. Due to the data loading bug, it now points to europe_bratislava.
    2. Call getZone("europe_bratislava")
      1. Check if europe_bratislava is a zone (false).
      2. Check if europe_bratislava is a link (true).
        1. Return early as the link check is more than 1 level deep.
  3. The result of getZone("europe_prague") is now null, even though the link is correct in the data.

In light of the above, is there a need to populate moment.tz._links with both directions of a link? I couldn't immediately see a use case for adding the reverse link, but it's very possible that I'm missing some context.

@ryandabler
Copy link

The bi-directional linking (with the potential overriding of a prior link due to the 1:many relationship of the "main" timezone and linked timezones) makes resolving what the main timezone is almost impossible.

When using the Intl.DateTimeFormat functionality, Chrome doesn't support timezones like US/Pacific-New while Firefox does. If moment timezone is set as US/Pacific-New I can use ._links to resolve up to America/Los_Angeles, but if the tz is already America/Los_Angeles then using the same ._links object will resolve to US/Pacific-New, and the routine breaks Chrome's date formatting.

It would be nice if the ._links object only was a sub-tz --> tz mapping and so accessing america_los_angeles in this object would return undefined and I can be sure I have a tz that will work with Chrome.

@ichernev
Copy link
Contributor

@gilmoreorless hm, this is a longstanding issue. Bidirectional linking makes some sense, but it should work more like equivalence sets than what it is now.

So if you have A --link-- B, you have one equivalence set (AB) with 2 participants. You can represent that with a consecutive number for equivalence set (like 0) and list of zones in it:

linkSetZones[0] = ['A', 'B'];
linkSet['A'] = 0;
linkSet['B'] = 0;

So when you need to add another link (say B --link-- C), you'd get

linkSetZones[0] = ['A', 'B', 'C'];
linkSet['A'] = 0;
linkSet['B'] = 0;
linkSet['C'] = 0;

And I'm guessing you can assume the first one in the list is the "main" / primary one.

You could represent it as a proper graph, but I fail to see the usefulness in that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants