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

[Bug]: AnnouncementTranslations has the wrong type signature #45

Closed
4 tasks done
hirasso opened this issue Apr 1, 2024 · 1 comment · Fixed by #46
Closed
4 tasks done

[Bug]: AnnouncementTranslations has the wrong type signature #45

hirasso opened this issue Apr 1, 2024 · 1 comment · Fixed by #46

Comments

@hirasso
Copy link
Member

hirasso commented Apr 1, 2024

What did you expect? 🧐

I added the announcements option as described in the readme:

{
  announcements: {
    'en-US': {
      visit: 'Navigated to: {title}',
      url: 'New page at {url}'
    },
    'de-DE': {
      visit: 'Navigiert zu: {title}',
      url: 'Neue Seite unter {url}'
    },
    '*': {
      visit: '{title}',
      url: '{url}'
    }
  }
}

What actually happened? 😵‍💫

When I do that, typescript complains since it expects the default visit and url parameters to exist at the top level of the option, like this:

{
  announcements: {
    'en-US': {
      visit: 'Navigated to: {title}',
      url: 'New page at {url}'
    },
    'de-DE': {
      visit: 'Navigiert zu: {title}',
      url: 'Neue Seite unter {url}'
    },
    visit: '{title}',
    url: '{url}'
  }
}

The implementation looks like it should work as described in the documentation. So we should probably change the type of the option from this:

/** Translations of announcements, keyed by language. */
type AnnouncementTranslations = {
	[lang: string]: Announcements;
} & {
	[key in keyof Announcements]: string;
};

To this:

/** Translations of announcements, keyed by language. */
type AnnouncementTranslations = {
	[lang: string]: Announcements;
};

Also, this looks redundant to me:

const lang = document.documentElement.lang || '*';

// @ts-expect-error: indexing is messy
const templates: Announcements = announcements[lang] || announcements['*'] || announcements;

lang has already a fallback of *, so we should be able to get away with this, shouldn't we?

const lang = document.documentElement.lang || '*';

// @ts-expect-error: indexing is messy
const templates: Announcements = announcements[lang] || announcements;

Overall it looks like this could use some cleanup. Happy to take care of it after #44 has landed, to avoid merge conflicts.

Swup and plugin versions 📝

  • @swup/a11y-plugin 4.5.0
  • swup 4.6.1

What browsers are you seeing the problem on? 🧭

No response

Relevant log output 🤓

No response

URL to minimal reproduction 🔗

n/a

Checked all these? 📚

@hirasso hirasso changed the title [Bug]: announcements is either wrong in the documentation or in the implementation [Bug]: AnnouncementTranslations has the wrong type signature Apr 1, 2024
@daun
Copy link
Member

daun commented Apr 1, 2024

@hirasso Nice catch. The types definitely look outdated. As to the template fallbacks, not sure if there was a specific edge case I had in mind or if it was just caution turning into paranoia 🤡 So yes, let's clean this up! Although I think we can do that independently of or even before that PR. Who knows if that code turns out to be a viable strategy, and improving the types shouldn't conflict with any of the work on that branch.

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 a pull request may close this issue.

2 participants