myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: Proposal: Get rid of shaded-impl module in core 2.1.x
Date Mon, 11 Jul 2011 16:47:41 GMT
Hi

I'll be clear and direct. What makes statements true or false are the
reasons behind it. Until this moment, you are not question the reasons
behind the reasoning proposed.

To be short, my argumentation is we can't change for now:

1. Everything inside org.apache.myfaces.spi
2. LifecycleProvided
3. Everything inside org.apache.myfaces.config.element

because JSF 2.1 is a maintenance release (not a mayor release) which
already has it first version (but even without a version). Do that
will create bugs on server integration code. The problem is there is
no way to detect such changes or create a proper patch from the server
side point of view without use some ugly reflection code and that
really s...cks!.

Let it to JSF 2.2 (which is another JSR) sounds better because in that
time I think we can get rid of implee6 too in one move!. In that
version, just change the poms, and move all code to impl and that's
it.

In conclusion, here we have a technical restriction that doesn't allow
us to move further, so we can't really make a vote because there is no
decision to make, we just can't change the code!. The idea of an API
in impl module is precisely make the "promise" that we will be nice
and do not change that stuff until the next major version.

Unfortunately there is nothing we can do in 2.1.x branch.

regards,

Leonardo Uribe

2011/7/11 Jakob Korherr <jakob.korherr@gmail.com>:
> Hi,
>
> No, sorry Leo, they are not enough. Frankly, I don't understand why
> you are blocking this solution. It is easy, it does not break anything
> (if we do not change the package names), it is a lot more clean and we
> can get rid of the shaded-impl module. If we don't do this now, we
> will maybe have to use this ugly module for a long time..
>
> And yes: in my opinion it is an epic fail. If you think otherwise,
> that's ok, but just because you say so and I don't does not make your
> statement true.
>
> I think we have to start a vote for this one just like we did with the
> resource-handler stuff.
>
> Regards,
> Jakob
>
> 2011/7/11 Leonardo Uribe <lu4242@gmail.com>:
>> Hi
>>
>> 2011/7/11 Jakob Korherr <jakob.korherr@gmail.com>:
>>> Hi,
>>>
>>>> 1. All classes in org.apache.myfaces.spi.
>>>
>>> I did not change anything here, just internal imports (from *.spi.impl
>>> to *.spi.util)!
>>
>> It is questionable to create .spi.util. After all, it is not supposed
>> that package be consumed by container integration code, so it should
>> be on spi.impl.
>>
>>>
>>>> 2. All classes that has to be with LifecycleProvider (@PostConstruct
>>>> annotation). Such classes should be on spi package, but note spi work
>>>> started after 2.0 release, so this should wait to a major version.
>>>
>>> Geronimo (for which we did the SPI stuff) includes MyFaces 2.0.x. Here
>>> we are talking about 2.1.x. Furthermore, one call to organize imports
>>> does the trick, so I do not see a problem here.
>>
>> Look the page of JSR-314 spec http://www.jcp.org/en/jsr/summary?id=314
>>
>> You will notice 2.1.x is not a new JSR like the future JSR-344 (JSF
>> 2.2), and instead is tagged as "Maintenance Release 2". So, to be
>> consistent, it should be possible to change between 2.0 and 2.1 on the
>> same server. That's the most important reason to be so conservative or
>> strict with 2.1.
>>
>>>
>>>> 3. All classes on org.apache.myfaces.config.element. These classes are
>>>> an interface to manipulate faces-config.xml files with java code, and
>>>> spi interfaces provide the hooks to get them, so in theory we can add
>>>> methods there, but relocate this package to
>>>> org.apache.myfaces.spi.data is not necessary. I think the package name
>>>> is correct.
>>>
>>> OK, fine. I thought the relocation to org.apache.myfaces.spi.* would
>>> fit better, since it is the myfaces-spi module and, again, since we're
>>> talking about 2.1.x, not 2.0.x here. But if you want to keep the
>>> package-name, I have no problem with that.
>>> org.apache.myfaces.config.element is fine too, however, you may not
>>> expect it to be in the myfaces-spi module.
>>>
>>>> [...] Considering this, I think the costs
>>>> involved on the changes proposed are too big compared with the
>>>> benefits, which are very limited and almost inexistent from user point
>>>> of view.
>>>
>>> From a user point of view: maybe yes. But from a developer point of
>>> view myfaces-shaded-impl is an epic fail. I know it was an easy an
>>> fast solution at the time we got 2.1.0 out, but for the long term this
>>> has to be changed. Please think about it, you use 2.0.5 (or any other
>>> _previous_ version) in myfaces-impl-ee6 as if it was 2.1.x-SNAPSHOT.
>>> Thus you use internals of previous versions which might not even be
>>> there anymore in the current 2.1.x-SNAPSHOT. And the worst part of it:
>>> you don't even see it at build time, b/c it's a separate module and
>>> myfaces-impl-ee6 is shaded into myfaces-impl (and shade does not warn
>>> about this kind of stuff, of course).
>>>
>>
>> I know the hack done to compile 2.1 is not very clean, but it works.
>> The idea is replace 2.0.5 ref with 2.1.0 in future versions. Note
>> myfaces-shaded-impl is a module that is just for allow compile
>> myfaces-impl-ee6, and nobody else. It is not "an epic fail", because
>> it does not cause any changes on the code that cause problems.
>>
>>> Considering this, it was ok to create shaded-impl in order to get the
>>> 2.1.0 release out, but for future releases (maybe also towards 2.2.0),
>>> this must be done in another way.
>>
>> In fact, the idea is do something in 2.2.x, but that will take some
>> time, so maybe we can keep in mind those changes until get there.
>>
>>> I have to say that I won't support a
>>> 2.1.2 release including the shaded-impl module.
>>
>> I hope my arguments could be enough to convince you about the opposite.
>>
>> regards ,
>>
>> Leonardo Uribe
>>
>>>
>>> Regards,
>>> Jakob
>>>
>>> 2011/7/10 Leonardo Uribe <lu4242@gmail.com>:
>>>> Hi Gerhard
>>>>
>>>> Ok, in theory the parts that we should not change, or otherwise that
>>>> will trigger a change on JEE containers are:
>>>>
>>>> 1. All classes in org.apache.myfaces.spi.
>>>> 2. All classes that has to be with LifecycleProvider (@PostConstruct
>>>> annotation). Such classes should be on spi package, but note spi work
>>>> started after 2.0 release, so this should wait to a major version.
>>>> 3. All classes on org.apache.myfaces.config.element. These classes are
>>>> an interface to manipulate faces-config.xml files with java code, and
>>>> spi interfaces provide the hooks to get them, so in theory we can add
>>>> methods there, but relocate this package to
>>>> org.apache.myfaces.spi.data is not necessary. I think the package name
>>>> is correct.
>>>>
>>>> regards,
>>>>
>>>> Leonardo Uribe
>>>>
>>>> 2011/7/9 Gerhard Petracek <gerhard.petracek@gmail.com>:
>>>>> hi leo,
>>>>> thx for checking it.
>>>>> it would be great, if you can post a list of parts (not classes) which
would
>>>>> break. so we can discuss this topic in a more concrete manner.
>>>>> regards,
>>>>> gerhard
>>>>> http://www.irian.at
>>>>>
>>>>> Your JSF powerhouse -
>>>>> JSF Consulting, Development and
>>>>> Courses in English and German
>>>>>
>>>>> Professional Support for Apache MyFaces
>>>>>
>>>>>
>>>>> 2011/7/10 Leonardo Uribe <lu4242@gmail.com>
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Good to know that. Unfortunately, do a change on the spi related
>>>>>> classes will break existing integration code with containers like
>>>>>> Geronimo, JBoss and others too. Considering this, I think the costs
>>>>>> involved on the changes proposed are too big compared with the
>>>>>> benefits, which are very limited and almost inexistent from user
point
>>>>>> of view.
>>>>>>
>>>>>> regards,
>>>>>>
>>>>>> Leonardo Uribe
>>>>>>
>>>>>> 2011/7/8 Werner Punz <werner.punz@gmail.com>:
>>>>>> > I have no problem if you break Ext-Script, after all  I program
against
>>>>>> > the
>>>>>> > impl, so whatever change you do I have to patch it in my codebase
>>>>>> > anyway.
>>>>>> > Don´t feel stopped by it.
>>>>>> >
>>>>>> > Werner
>>>>>> >
>>>>>> >
>>>>>> > Am 08.07.11 18:27, schrieb Gerhard Petracek:
>>>>>> >>
>>>>>> >> hi,
>>>>>> >>
>>>>>> >> thx for reviewing the changes.
>>>>>> >> for sure we can discuss when we are going to do such changes.
however,
>>>>>> >> if ext-script is the only reason against it, we can do it
right now
>>>>>> >> imo.
>>>>>> >> there is no problem with shipping a small update of ext-script
and the
>>>>>> >> user base is currently small enough to communicate this
change easily.
>>>>>> >>
>>>>>> >> regards,
>>>>>> >> gerhard
>>>>>> >>
>>>>>> >> http://www.irian.at
>>>>>> >>
>>>>>> >> Your JSF powerhouse -
>>>>>> >> JSF Consulting, Development and
>>>>>> >> Courses in English and German
>>>>>> >>
>>>>>> >> Professional Support for Apache MyFaces
>>>>>> >>
>>>>>> >>
>>>>>> >>
>>>>>> >> 2011/7/8 Leonardo Uribe <lu4242@gmail.com <mailto:lu4242@gmail.com>>
>>>>>> >>
>>>>>> >>    Hi
>>>>>> >>
>>>>>> >>    In principle I agree with this change, but after a
quick patch
>>>>>> >> review
>>>>>> >>    I saw some package relocation for some classes (from
>>>>>> >>    org.apache.myfaces.config.element to org.apache.myfaces.spi.data).
>>>>>> >>    Those changes will break extensions scripting code.
Such changes
>>>>>> >>    should be done between major versions (2.2.0). Please
do not change
>>>>>> >>    the package names.
>>>>>> >>
>>>>>> >>    regards,
>>>>>> >>
>>>>>> >>    Leonardo Uribe
>>>>>> >>
>>>>>> >>    2011/7/8 Gerhard Petracek <gerhard.petracek@gmail.com
>>>>>> >>    <mailto:gerhard.petracek@gmail.com>>:
>>>>>> >>     > +1
>>>>>> >>     > regards,
>>>>>> >>     > gerhard
>>>>>> >>     >
>>>>>> >>     > http://www.irian.at
>>>>>> >>     >
>>>>>> >>     > Your JSF powerhouse -
>>>>>> >>     > JSF Consulting, Development and
>>>>>> >>     > Courses in English and German
>>>>>> >>     >
>>>>>> >>     > Professional Support for Apache MyFaces
>>>>>> >>     >
>>>>>> >>     >
>>>>>> >>     >
>>>>>> >>     > 2011/7/8 Jakob Korherr <jakob.korherr@gmail.com
>>>>>> >>    <mailto:jakob.korherr@gmail.com>>
>>>>>> >>     >>
>>>>>> >>     >> Hi guys,
>>>>>> >>     >>
>>>>>> >>     >> We currently use the shaded-impl module in
core 2.1.x to solve a
>>>>>> >>     >> cyclic dependency problem. However, this
introduces redundancy
>>>>>> >> and
>>>>>> >>     >> complexity and may lead to some strange issues.
This can be
>>>>>> >>    solved by
>>>>>> >>     >> the introduction of a myfaces-spi module
(which is then packed
>>>>>> >>     >> together with myfaces-impl, just like myfaces-impl-ee6
is).
>>>>>> >>     >>
>>>>>> >>     >> Please see MYFACES-3211 [1] for a detailed
description of the
>>>>>> >>    solution
>>>>>> >>     >> and a patch for it.
>>>>>> >>     >>
>>>>>> >>     >> If there are no objections, I will commit
this patch soon!
>>>>>> >>     >>
>>>>>> >>     >> Regards,
>>>>>> >>     >> Jakob
>>>>>> >>     >>
>>>>>> >>     >> [1] https://issues.apache.org/jira/browse/MYFACES-3211
>>>>>> >>     >>
>>>>>> >>     >> --
>>>>>> >>     >> Jakob Korherr
>>>>>> >>     >>
>>>>>> >>     >> blog: http://www.jakobk.com
>>>>>> >>     >> twitter: http://twitter.com/jakobkorherr
>>>>>> >>     >> work: http://www.irian.at
>>>>>> >>     >
>>>>>> >>     >
>>>>>> >>
>>>>>> >>
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Jakob Korherr
>>>
>>> blog: http://www.jakobk.com
>>> twitter: http://twitter.com/jakobkorherr
>>> work: http://www.irian.at
>>>
>>
>
>
>
> --
> Jakob Korherr
>
> blog: http://www.jakobk.com
> twitter: http://twitter.com/jakobkorherr
> work: http://www.irian.at
>

Mime
View raw message