Format sources properly on country panel using Intl.ListFormat

[VIKTORVAV99]

This formats the sources on the country panel properly while also localizing the strings.

Reference: #4362

[Kongkille]

Hmm this would require splitting and formatting on each render.

Perhaps I also miscommunicated in my last thread, when I said multiple items were depending on this value, I was referring to the entire infrastructure and DB, so we just can't change it to an array in all projects and the database.

I think that an arguably better solution would be to change this line here so we always treat source as an array of sources.

https://github.com/electricitymap/electricitymap-contrib/blob/200024b9ffe7a2f291020dc401563888bd3bf8dd/web/src/reducers/dataReducer.js#L74

Then we can at least avoid splitting on each render.

Additionally maybe we'd want to use a memo for the formatted string, I can't recall the exact ratio for when a memo is worth it, but I believe we rerender the panel each time the graph/time is changed, while we don't actually need the sources to change

PS: I know the actual performance changes of this is not very high, but I think it's still worth thinking about

[VIKTORVAV99]

Hmm this would require splitting and formatting on each render.

Perhaps I also miscommunicated in my last thread, when I said multiple items were depending on this value, I was referring to the entire infrastructure and DB, so we just can't change it to an array in all projects and the database.

I think that an arguably better solution would be to change this line here so we always treat source as an array of sources.

https://github.com/electricitymap/electricitymap-contrib/blob/200024b9ffe7a2f291020dc401563888bd3bf8dd/web/src/reducers/dataReducer.js#L74

Then we can at least avoid splitting on each render.

Additionally maybe we'd want to use a memo for the formatted string, I can't recall the exact ratio for when a memo is worth it, but I believe we rerender the panel each time the graph/time is changed, while we don't actually need the sources to change

PS: I know the actual performance changes of this is not very high, but I think it's still worth thinking about

Oh, yeah it would be better to do it further upstream as you suggest, as for memo I can certainly add it but not sure if it will be worth it with the above change.

I'll see if I can update the removeDuplicateSources() function directly but I don't think I'll have the time today.

EDIT: Turning this into a draft in the meantime.

[VIKTORVAV99]

I ended up changing the removeDuplicateSources function ever so slightly so it return a array instead of a string.

I couldn't find any other places where "data".source was used so it should be safe.

[VIKTORVAV99]

This change is needed so Intl.ListFormat don't fail.

[Kongkille]

Looks great,

I am not sure in general how to handle the new keyword with intl stuff. We also use the new keyword elsewhere on each render so it's not exclusive here.

I am not sure if that could affect performance.

[Kongkille]

Actually I just forgot, we have a file called formatting.js

I think it would be pretty cool if we moved this into a function called "formatDataSources" and placed it there

[VIKTORVAV99]

Actually I just forgot, we have a file called formatting.js

I think it would be pretty cool if we moved this into a function called "formatDataSources" and placed it there

I'll try and move them later today. 👍🏼

[VIKTORVAV99]

Looks great,

I am not sure in general how to handle the new keyword with intl stuff. We also use the new keyword elsewhere on each render so it's not exclusive here.

I am not sure if that could affect performance.

I think we should treat Intl itself just like Math or Object but since the Intl objects consume configurations (settings) we could define those in a "settings" file/object of some kind.

[Kongkille]

Looks great,
I am not sure in general how to handle the new keyword with intl stuff. We also use the new keyword elsewhere on each render so it's not exclusive here.
I am not sure if that could affect performance.

I think we should treat Intl itself just like Math or Object but since the Intl objects consume configurations (settings) we could define those in a "settings" file/object of some kind.

Yeah, specifically i am thinking that for each render we create a new instance of the intl object. This has some kind of instantiation cost associated to it. Anyways, that's probably not relevant for this PR

[VIKTORVAV99]

I didn't have time to change anything today but I'll do it this weekend.

However I think this will suffer the same fate as #4630 so I was thinking of just adding a fallback to display a simple array (like today) instead if Intl.ListFormat is missing, what do you think @madsnedergaard?

[madsnedergaard]

I didn't have time to change anything today but I'll do it this weekend.

However I think this will suffer the same fate as #4630 so I was thinking of just adding a fallback to display a simple array (like today) instead if Intl.ListFormat is missing, what do you think @madsnedergaard?

Sounds good to me! :)

[VIKTORVAV99]

@Kongkille should be good to go now! I created a formatting function and fixed the merge conflicts.

[madsnedergaard]

Great! I'll create a PR for fixing #4630 :)

Requested change has been fixed already :)

[madsnedergaard]

I just tried the "dismiss review" feature, but it's probably best to avoid it in future in case @Kongkille had some additional comments to the implementation of the requested change 😄

[VIKTORVAV99]

Maybe I should have held off on the merge then... 😅
But if you got any input @Kongkille I'd be happy to address it in a new PR as soon as possible.