search

liferay / liferay-frontend-projects

A monorepo containing assorted Frontend Infrastructure Team projects
Other
66 stars 67 forks source link

feat(js-api)!: improve FDS filter CX interfaces #1172

Closed izaera closed 9 months ago

izaera commented 9 months ago

See https://liferay.atlassian.net/browse/LPS-197299

This is a breaking change but we have not yet released any GA version of @liferay/js-api so there shouldn't be any problem.

Note that I've changed the names of the exported functions and types to meaningful (from the POV of the customer) ones.

This is because I think we should not tie API names to portal internal stuff. In this case I've left the HTMLBuilder concept around because it makes sense, but I've renamed the names of the methods and they don't match the internal ones we use in portal any more.

The rationale for not aligning API and internal names is simple: internal names are subject to change and do change, in fact, as we evolve the product, whereas API names must remain stable forever unless we want to be continuously breaking customers code.

izaera commented 9 months ago

After we agree on API names, I'll send a PR for liferay-portal to be updated with the new contract.

izaera commented 9 months ago

The two comments related of types.ts in DXP makes me think that the whole content of that file should really be here. Maybe in a separate file, not in data-set dir, as they are not directly related to FDS. What do you think?

At first sight I would have bought your argument, but then I reflected a bit more and now I'm not sure...

On the one hand, if this project was internal to portal (or to any other project) I would do what you say. But my intuition tells me that we should treat @liferay/js-api more as a spec than as code. In other words, think about the HTTP spec: it explains how things should work not how they should be implemented.

Modeling a FDSCellRenderer (or an FDSFilter) as an extension of an abstract type Renderer is a "meta-implementation" detail. I added "meta" to convey that it's not that we are implementing something, because these are only types and we are modelling, but we are imposing a restriction on the model itself: we are forcing how to define/interpret the model.

Seen from the POV of the portal it may make a lot of sense to have a common Renderer type, but seen from the POV of a customer who's writing a cell renderer, why should he care?

I mean, having a clear spec of what is a FDSCellRenderer is more than enough, no matter if it could share fields with or look like an FDSFilter and no matter if the portal treats them internally as the same type object, which makes sharing the fields not a coincidence but a real thing.

Also, think of what would happen if in the future we decide to change how FDSCellRenderers are implemented but also want to leave FDSFilter untouched. Or imagine that we stop treating these two types as subtypes of the same entity Renderer (in portal)...

That said, there is an scenario where a supertype Renderer could make sense in @liferay/js-api and it would be the case of customers making complex CX collections and writing code to manage all of them. In that case it could be handy for a customer to have a Renderer type but even in this case we could argue that if they want it, they can create it in their project, since that is possible in TypeScript (not in other languages, but we are lucky this time :sweat_smile: ).

So to sum up:

  1. I would make @liferay/js-api minimal in the sense of not creating complex hierarchies of types and not creating any type that is not absolutely necessary.
  2. I would treat each CX spec as isolated from the others, even if they share fields and/or are the very same thing in portal.

I'm 100% sure of :one: .

I'm open to discussing :two: a bit more since I admit there's some risk of bloating @liferay/js-api with very similar types. I suppose that the problem here is placing the border line between what definitely lies in :one: and what doesn't...

izaera commented 9 months ago

I'm open to discussing 2️⃣ a bit more since I admit there's some risk of bloating @liferay/js-api with very similar types. I suppose that the problem here is placing the border line between what definitely lies in 1️⃣ and what doesn't...

The good news is that we don't need to create supertypes from the very beginning, we can introduce them in the future if we see they finally make sense and we are committed to them in the long term.

markocikos commented 9 months ago

A lot of good points there, @izaera! :sparkles: I don't have much to add, except that I completely agree.