Chore: add accessor to formatters

[etienneburdet]

Summary

The goal for this PR is to add accessor functions to table fields, in order to extract the value from a more complex datasource. Overall it decouples what we pass as value to what we pass to dsiplay. It's needed for files on the platform, will be needed to display label on geocolumns and could be used for links etc.

(Internal for Opendatasoft only) Associated Shortcut ticket: sc-50639.

Changes

The main change is to add an accessor functions, that transform rawValues into the value of the cell–which can now be different from it's display.
Other possibility: Tanstack table has accessorKey and accessorFn to quickly access another column. I though for our case, accessorFn would be general enough.

I also added an option to enable debug warning, because my console was exploding on the platform side 💥

Changelog

"We added accessor function to formatter (and thus table cells) so that it's possible to more complex object and extract value, e.g. {href, tilte}."

Review checklist

• Description is complete
• Commits respect the Conventional Commits Specification
• 2 reviewers (1 if trivial)
• Tests coverage has improved
• Code is ready for a release on NPM

[KevinFabre-ods]

I struggle to understand the difference between display and accessor for some formats.
To me, it's the same thing for Boolean, Short and Long text formats.

Can't we find another wording for this property? (like transformer?) and only defined it where it make sense.

• DateFormat
• NumberFormat
• URLFormat
• GeoFormat

Looking at the TanStack documentation, the accessorfn has the record item as 1st parameter.
So It can be used to build something like this:

item => `${item.first} ${item.lastName}`

In our case, the accessor only get the raw value as parameter.
As I said early maybe the wording is not quite right.

---

For complex formats such as DateFormat, NumberFormat, URLFormat, or GeoFormat, it makes sense to separate how the 'label' is constructed from how the raw value is used to render a date, number, URL link, or map.
However, something still feels off to me. Perhaps the comment I made earlier regarding code reorganization, particularly around type checks and warning triggers, could help make this distinction clearer.

[etienneburdet]

So I changed things a bit so that: 

• A column has an accessor for its value of form fullRecord => value
• Options can be either an object or an accessor of form fullRecord => optionsObject
• Formatters don't take accessors, but some options still are function of the rawValue.
• I think it makes sense to have display mutating the value after everything (or else why using a formatter?). sources is very likely to be a function too, since rawValue will be a shape. In all cases it's always possible to fallback. to the accessor and make things that depend on the whole record.

I changed the geosshape story a bit, so that we make use of that to label the geofield with another column. I didn't implement "hidden" column option, but that's the spirit I think.

[KevinFabre-ods]

Hello,

Could you try to update conditions where the value type is checked?
Like we agreed in #265 (comment)

[etienneburdet]

Random naming though: value/valueToString instead of rawValue/display ?

[KevinFabre-ods]

Random naming though: value/valueToString instead of rawValue/display ?

Yes, that would be better.
Maybe valueToLabel valueToText is better than valueToString to clearly indicates that is will transform the label into something readable for a human

[KevinFabre-ods]

Great job to extract the format logic 👍

I agree on the data flow from the table to the format components.

• accessor is for a table column to override the value we pass to a format component ✅
• column.options is either a function (that take the record data) or an object ✅
  • options.rawValue is however required 🔴 For a column table, rawValue comes from column.key or column.accessor
• In format component, most props are primitive or object. If we have props that are function that take the result of the formatting as parameter ✅

I'm also in favor to rename rawValue

[KevinFabre-ods]

There are things to fixed with the debug data flow

Main issue is for me the typescript errors, as it stands, we'll get these errors too in our platform repository

[etienneburdet]

Ok so I fixed the options import by not using the path alias… they don't work with types between the two repos for now.

As for typing the accessor functions, I resorted to some any: doesn't yell at you if you don't input anything, but you can type the columns if you want. Combinations of never ? … : or R = unknown just forced to add a generic all the way up to TableProps which was pretty alien.

[KevinFabre-ods]

I think we are done with all the new accessor logic. GG! 👏

Could you please check my comments in my previous reviews, IMO we still need to resolve issues / suggestions

[KevinFabre-ods]

👋

With your latest commit, I was able to resolve some of comments.

Could you give your thoughts about the comments that are still opened ?

• debugs warning are not thrown in the console Chore: add accessor to formatters #267 (comment)
• addition of the debugWarnings property can be simplified. there is one ternary condition too many Chore: add accessor to formatters #267 (comment)
• Unused variables Chore: add accessor to formatters #267 (comment)
• Should the TextFormat have a span tag ? Chore: add accessor to formatters #267 (comment)

[KevinFabre-ods]

GG WP! 👏

[richterb]

The changes make sense! I added a small remark regarding the debug logger but it's minor and probably impossible to do 🙈

[richterb]

In general, better to avoid upgrades (especially a major version) in a feature PR 😄 But if you tested alright and Kevin and I didn't get any issue during review either, I guess we're good

[etienneburdet]

It's for github actions, it crashes because nx doesn't find its arm64 or linux package… It's not the first time and I really don't know how to fix that. Maybe we should have a fixed version instead of carret 🤔

[richterb]

Agreed. I once had a similar issue locally, and then not anymore a few days later, so that's a good probability 😬

[richterb]

Usually I tend to add the "if (debugWarning)" in the debug function itself, so that future devs that use it don't have to remember to do the check themselves. Also it prevents having to pass debugWarning deep down. But I'm not sure that the store that holds the value is accessible here :/ My Svelte is rusty

[etienneburdet]

We have to subscribe manually, but debugWarnings store could be used in utils.ts indeed. (it changes in svelte 5 with .svelte.ts files).

[wmai]

I'm currently getting familiar with the Table component code, but at first glance, everything looks great 👏

I've only one small question regarding the GeoFormat, when valueToLabel is undefined, nothing is displayed. Is this intentional? Wouldn't it be preferable to default to displaying the coordinates as a simple string?