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

Comprehensive date conversion #2093

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

enellis
Copy link
Contributor

@enellis enellis commented Nov 13, 2024

See discussion at #2080.

Summary

This PR introduces a mapping of Japanese era dates to Gregorian calendar dates, enabling comprehensive date conversion.

It includes accurate calculation of the time span of years and months for periods predating the adoption of the Gregorian calendar in Japan.
Additionally, day-to-day conversion of Japanese era dates to Gregorian calendar dates was added.


Fixes #2080.

@birtles
Copy link
Member

birtles commented Nov 14, 2024

This is amazing 🤩

I'll have a closer look after lunch but at first glance the main concern I have is that the content in era-info.ts ends up in the content script which means we inject it into every page. It might be nice if we could either load it lazily or, more likely, postMessage to the background script and have it convert the date.

src/content/dates.ts Outdated Show resolved Hide resolved
@birtles
Copy link
Member

birtles commented Nov 14, 2024

I'll have a closer look after lunch but at first glance the main concern I have is that the content in era-info.ts ends up in the content script which means we inject it into every page. It might be nice if we could either load it lazily or, more likely, postMessage to the background script and have it convert the date.

I had a bit more of a look and it really looks great. Thank you so much for doing this.

I think it really would be nice if we can somehow get the era-info to live in the background script but I can see that's going to be tricky because:

  1. We need the era names in isEraName which I believe needs to be part of the content script.
  2. It introduces more async behavior which means we need to think about things like handling interrupted requests.

I think (2) might be easier to manage once we have converted the rest of the popup to Preact (since we could wrap up all that async state in its own component) but I'm not sure we want to block merging this on that work.

Maybe for now we can sneak in an extra browser.runtime.sendMessage in lookupText? Or even in query itself by chaining it to the rawWordsQuery?

But perhaps we should start by measuring how much that content script actually grows by including era-info? Perhaps I'm just overreacting?

@enellis
Copy link
Contributor Author

enellis commented Nov 14, 2024

Thank you for having a closer look! To be honest, I didn't really have any good ideas how to integrate this reasonably large dataset in a nice way into the project, so I'm happy about any feedback in that regard.

But perhaps we should start by measuring how much that content script actually grows by including era-info? Perhaps I'm just overreacting?

According to the compiler output it grows from 664.396 KiB to 1.269 MiB, nearly doubling in size. So it's probably really not a good idea to keep the data there.

  1. We need the era names in isEraName which I believe needs to be part of the content script.

I guess, keeping just the era names in a separate Set in the content script would be okay.

2. It introduces more async behavior which means we need to think about things like handling interrupted requests.

I think (2) might be easier to manage once we have converted the rest of the popup to Preact (since we could wrap up all that async state in its own component) but I'm not sure we want to block merging this on that work.

Maybe for now we can sneak in an extra browser.runtime.sendMessage in lookupText? Or even in query itself by chaining it to the rawWordsQuery?

Personally, I would be fine with waiting a little longer and integrating it when the time is right, which would save us from writing potentially hacky code that is going to be removed anyway. However, if you think your proposed solution is easy enough to implement, I'd be happy to give it a try.

@birtles
Copy link
Member

birtles commented Nov 15, 2024

But perhaps we should start by measuring how much that content script actually grows by including era-info? Perhaps I'm just overreacting?

According to the compiler output it grows from 664.396 KiB to 1.269 MiB, nearly doubling in size. So it's probably really not a good idea to keep the data there.

Yes, that does sound like a bit too much.

  1. We need the era names in isEraName which I believe needs to be part of the content script.

I guess, keeping just the era names in a separate Set in the content script would be okay.

I agree—I don't think the set of eras is going to change frequently enough that we need to worry about duplicating that information!

  1. It introduces more async behavior which means we need to think about things like handling interrupted requests.
    I think (2) might be easier to manage once we have converted the rest of the popup to Preact (since we could wrap up all that async state in its own component) but I'm not sure we want to block merging this on that work.
    Maybe for now we can sneak in an extra browser.runtime.sendMessage in lookupText? Or even in query itself by chaining it to the rawWordsQuery?

Personally, I would be fine with waiting a little longer and integrating it when the time is right, which would save us from writing potentially hacky code that is going to be removed anyway. However, if you think your proposed solution is easy enough to implement, I'd be happy to give it a try.

Thanks for being so patient. In that case I guess I should probably bump up the priority of the Preact conversion work a notch.

Thank you again!

@enellis
Copy link
Contributor Author

enellis commented Nov 15, 2024

Thanks for being so patient. In that case I guess I should probably bump up the priority of the Preact conversion work a notch.

As I really only want to mess with your priorities and schedule as little as possible, I would offer converting the metadata component myself. If I understand correctly, one can control very fine-grained what parts are using Preact. The problem is, I've never came into contact with React (and Tailwind) before. (Until now, I've been using exclusively Svelte for my reactivity needs.) So, I can't say whether I'll succeed or whether, in the end, it will be more work for you, caused by the need of the reviews being more extensive and detailed than usual. Let me know what you think about that.

Aside from that, I've got some general questions about the transition to Preact:

  • I suppose the only way firing up cosmos currently is using a node version that still supports the --experimental-policy flag?
  • If I see correctly, currently we solely use preact/hooks for managing reactive state. Personally, I would be more inclined to use preact/signals. Would it be okay for you to mix both approaches in this project?

@birtles
Copy link
Member

birtles commented Nov 20, 2024

As I really only want to mess with your priorities and schedule as little as possible, I would offer converting the metadata component myself. If I understand correctly, one can control very fine-grained what parts are using Preact.

In a sense. We've been doing a kind of bottom-up conversion. So we started by converting just one part of the kanji tab, and then its direct sibling, and gradually expanding until we covered the whole kanji entry.

I imagine the next step would be to take the name entries and convert them over. Then word entries, then meta data, and then start to stitch it all together so we only have one root render() call.

The problem is, I've never came into contact with React (and Tailwind) before. (Until now, I've been using exclusively Svelte for my reactivity needs.) So, I can't say whether I'll succeed or whether, in the end, it will be more work for you, caused by the need of the reviews being more extensive and detailed than usual. Let me know what you think about that.

I think you'll find it's not too hard, especially the name entries as they're quite simple and have almost no interactivity.

The hardest part is probably the Tailwind part since the setup we're using is a bit funky for two reasons:

  1. In order to prevent the options styles clashing with the popup styles (long story), we prefix the Tailwind classes with tp- (tenten popup). It's really easy to forget the prefix and wonder why it doesn't seem to work and sometimes the prefix goes in a different place.

  2. We define a lot of measurements relative to var(--base-font-size) (e.g.). We do this instead of using rem units so that we're not affected by the host page's base font size. There's a checkbox in the React Cosmos fixture to test this: when you toggle it nothing should change.

    The tricky thing is we have been adding these size definitions to the Tailwind config file on an as-needed basis and in order to convert existing measurements from pixels to these definitions you often need to do a bit of calculation and then pick the closest option (and just eyeball it to see if it looks reasonable).

I often end up making the component support both Tailwind and the old styles initially and render both options side-by-side to check if they look the same. They don't need to be identical but they should be close.

Aside from that, I've got some general questions about the transition to Preact:

  • I suppose the only way firing up cosmos currently is using a node version that still supports the --experimental-policy flag?

Oh yeah, that's a major problem. I tried a bunch of ways to get React Cosmos to work with Preact but that was the only one that I could get to work at the time. I recently discovered that --experimental-policy has been removed from Node but I've yet to find an alternative solution that works. You can see some of the approaches I tried here: react-cosmos/react-cosmos#1552

  • If I see correctly, currently we solely use preact/hooks for managing reactive state. Personally, I would be more inclined to use preact/signals. Would it be okay for you to mix both approaches in this project?

I'm very interested in preact/signals but I haven't used it yet because it increases the bundle size if we also use hooks. I didn't actually realize they can completely replace hooks. If you want to try replacing our existing hooks with signals I'd be very interested to see the result!

@enellis
Copy link
Contributor Author

enellis commented Nov 22, 2024

Thank you so much for all this very helpful information!

I'm very interested in preact/signals but I haven't used it yet because it increases the bundle size if we also use hooks. I didn't actually realize they can completely replace hooks. If you want to try replacing our existing hooks with signals I'd be very interested to see the result!

I started with replacing all hooks with signals in the content script here. I really like using them, managing global app state becomes super easy. The bundles size of the content script still increased about 12KiB, though. It seems like using the signal wrappers for use in components (e.g. useSignal(), so that the signals are memoized) still make calls to the hook API under the hood. I'd be happy if you could take a look and share your thoughts.

I imagine the next step would be to take the name entries and convert them over. Then word entries, then meta data, and then start to stitch it all together so we only have one root render() call.

I'm going to start looking into that now :)

@birtles
Copy link
Member

birtles commented Nov 26, 2024

I started with replacing all hooks with signals in the content script here. I really like using them, managing global app state becomes super easy. The bundles size of the content script still increased about 12KiB, though. It seems like using the signal wrappers for use in components (e.g. useSignal(), so that the signals are memoized) still make calls to the hook API under the hood. I'd be happy if you could take a look and share your thoughts.

Thanks! Sorry I've been a little busy recently but I'll look into the two draft PRs in the next couple of days. 12KiB sounds reasonable to me though.

I imagine the next step would be to take the name entries and convert them over. Then word entries, then meta data, and then start to stitch it all together so we only have one root render() call.

I'm going to start looking into that now :)

Thank you!

By the way, did you work out how to get React Cosmos to run on more recent versions of Node?

@enellis
Copy link
Contributor Author

enellis commented Nov 26, 2024

Thanks! Sorry I've been a little busy recently but I'll look into the two draft PRs in the next couple of days. 12KiB sounds reasonable to me though.

No worries, take your time!

By the way, did you work out how to get React Cosmos to run on more recent versions of Node?

Unfortunately not. I tried a few different aliasing approaches, but nothing worked, so I stuck with using Node v20 for now.

@enellis enellis force-pushed the date-conversion branch 3 times, most recently from 4de1b83 to 380c82f Compare December 2, 2024 00:11
A mapping of Japanese era dates to Gregorian calendar dates,
created using conversion tables sourced from Japanese
Wikipedia articles.
The file is renamed to better reflect its expanded scope,
as it is going to support comprehensive date conversions,
not just year-specific functionality.
This commit starts using the mapping of Japanese era dates
to Gregorian calendar dates, enabling comprehensive date
conversion.

It includes accurate calculation of the time span of years and
months for periods predating the adoption of the Gregorian
calendar in Japan.
Additionally, day-to-day conversion of Japanese era dates
to Gregorian calendar dates was added.
@enellis enellis force-pushed the date-conversion branch 2 times, most recently from 5a24a23 to 0883d02 Compare December 2, 2024 11:54
@enellis
Copy link
Contributor Author

enellis commented Dec 2, 2024

Everything depending on the era-info.ts dataset was moved to the background script. The pure year offset calculation for Gregorian eras still takes place in the content script.

I currently have two main concerns:

  • We now cannot know in the content script whether a pre-Gregorian date is valid or not, e.g. 安政8年 is invalid, as the Ansei era were only 7 years. Invalid dates of this kind will be parsed and selected; I decided for the time being to display 安政8年 = no result in cases like these.
  • The popup completely rerenders when anchoring after a few seconds. During that rerender there is a noticeable flickering, as it fetches the data from the background script again. This is not the case for pure year offset calculations, as we don't have to communicate with the background script there. This problem hopefully will be gone as soon as the whole popup is converted to Preact and we don't have to do complete rerenders anymore. Is there anything we can do about this now, though?
flickering.mov

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 this pull request may close these issues.

reign names calculator off by one year
2 participants