birchill / 10ten-ja-reader

A browser extension to translate Japanese by hovering over words.
https://addons.mozilla.org/firefox/addon/10ten-ja-reader/
GNU General Public License v3.0
604 stars 45 forks source link

Comprehensive date conversion #2093

Open enellis opened 4 days ago

enellis commented 4 days ago

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 commented 4 days ago

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.

birtles commented 4 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 2 days ago

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: