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.

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?

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.

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!

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.

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!

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?

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!

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 :)