faces icon indicating copy to clipboard operation
faces copied to clipboard

StateHelper: make methods generics aware

Open tandraschko opened this issue 1 year ago • 18 comments

like

public Object eval(Serializable key, Object defaultValue);

to

public <T> T eval(Serializable key, T defaultValue);

tandraschko avatar Mar 12 '25 12:03 tandraschko

https://github.com/apache/myfaces/commit/a58cb8c3a731ff5efe9a5e7f8523aacd02eeb41b

tandraschko avatar Mar 12 '25 12:03 tandraschko

Where is the discussion? And what are the downsides?

mnriem avatar Mar 21 '25 21:03 mnriem

Do we need a discussion for it? IMO it just makes the API generics aware, which is done like 10 years to late.

tandraschko avatar Apr 07 '25 09:04 tandraschko

+1

BalusC avatar Apr 15 '25 10:04 BalusC

+1

at the moment the only solution is to use the OmniFaces State wrapper!

pizzi80 avatar Apr 30 '25 16:04 pizzi80

I can't generify Object eval(Serializable key, Supplier<Object> defaultValueSupplier); as this makes existing impls incompatible. E.g. ComponentStateHelper doesn't anymore compile after this change. This thus breaks backwards compatibility. The correct procedure is to add a new method (with different name) and deprecate the old method with a (javadoc) reference to the new method.

Can you perhaps propose a suitable name for the new method or a working/acceptable alternative?

BalusC avatar Aug 05 '25 15:08 BalusC

Imo we can just break it? No applications use a custom one and its a Migration to 5.0

tandraschko avatar Aug 05 '25 17:08 tandraschko

Yeah, but it's a public API and therefore the JEE spec committee won't accept this .. The correct procedure is deprecation.

Unless there's an alternative I'm not aware of, @arjantijms?

BalusC avatar Aug 06 '25 11:08 BalusC

AFAIR we have already breaking changes like making FacesMessage.Severity to enum? if a user weirdly uses this, it might break already

tandraschko avatar Aug 06 '25 11:08 tandraschko

AFAIR we have already breaking changes like making FacesMessage.Severity to enum? if a user weirdly uses this, it might break already

The enum usage is 100% backwards compatible with existing code syntax for class constants. And, incompatible methods have been marked deprecated for removal with a reference to the new/correct approach.

BalusC avatar Aug 06 '25 12:08 BalusC

hm i see

i just would like to add that i maintain libs and projects around JSF since 15 years and never seen a custom StateHelper

and it would be a shame to using a different name just because we introduce generics like 10 years to late.

tandraschko avatar Aug 06 '25 12:08 tandraschko

Fully agree.

Hence, @arjantijms do you know a backdoor which would allow us making this compile-time incompatible change? Or perhaps @mnriem?

To summarize, we'd like to change in StateHelper

Object eval(Serializable key, Supplier<Object> defaultValueSupplier);

to

<T> T eval(Serializable key, Supplier<T> defaultValueSupplier);

but then existing impls won't compile anymore and developer has to edit these.

BalusC avatar Aug 06 '25 13:08 BalusC

I don't get the problem ...

if a developer upgrade to Faces 5 and in a custom component there is a getter like this:

public String getLocale() { return (String) getStateHelper().eval(PropertyKeys.locale); }

the fact that now the StateHelper returns a String do not create any compilation problem...

the cast become superfluos and it could be removed

pizzi80 avatar Sep 24 '25 14:09 pizzi80

The "jakarta faces end user" usage will be unaffected indeed. But we're talking about the implementation of the StateHelper interface. I.e. the actual code of whatever instance is returned by getStateHelper() method. This could have been customized (although super rare but not impossible). And this change would break it. This existing custom implementation would not anymore compile and would need editing. And this goes against the Jakarta EE spec rules wrt backwards compatibility.

BalusC avatar Oct 18 '25 10:10 BalusC

isnt this just a theoretical problem? PrimeFaces and OmniFaces doesnt have it, every other lib isnt maintained for years?!

tandraschko avatar Oct 18 '25 13:10 tandraschko

IMHO ... just talking ...

probably they are 2 o 3 devs in the world...

they could stay in 4.1 line and when they are ready to upgrade probably with some minutes of work they can improve their code...

pizzi80 avatar Oct 20 '25 10:10 pizzi80

I fully agree but I don't control JEE spec rules.

According to the current JEE spec rules, such change would block the inclusion of Faces 5.0 in JEE 12.

cc: @arjantijms -- is there really no way we can control/force this..??

BalusC avatar Oct 20 '25 10:10 BalusC

I know, I know... ;)

Some of these "rules" are probably very outdated because the software industry moves 10 times faster today than it did 15 years ago.

Also the IDEs are very powerful and there is github ... it's a different story...

pizzi80 avatar Oct 20 '25 10:10 pizzi80

Implemented except for Object eval(Serializable key, Supplier<Object> defaultValueSupplier) due to backwards compatibility.

BalusC avatar Jan 09 '26 09:01 BalusC

Reopening because backwards incompatibility is no issue for major version (as long as it's in changelog) and we now have control over old-TCK.

BalusC avatar Jan 13 '26 11:01 BalusC