Feature/myfaces 4466 faces.js reimplementation (target 4.0.0 RC3)

[werpu]

hat to recreate the pull request because i had to rename the branch

[werpu]

@melloware I just had to rename the branch, I had accidentally added the wrong jira issue reference. Please reconfirm

[werpu]

I will squash the commits at the final merge into master, the so the accidentally referenced myfaces-4456 commits will be squashed into one single big commit with a myfaces-4466 reference. Renaming atm is not possible anymore due to rebase problems.

[werpu]

I will revisit the topic regarding the mapping files again. Atm we have solved that by adding a special extension which allows if mapped to the faces servlet to load the map file for faces-development. Given I want mapping to be optional, I will revisit this issue again, I probably will provide a special servlet filter on the impl side, which adds the mapping file data automatically in the js file depending on the faces servlet setting, if wanted.

I will open a jira enhancement issue for this and will provide the code.

[werpu]

4487 is in, I had to alter the resource loader call chain slightly and have introduce one which does the mapping file referencing upon load time. Please test properly whether this raises any issues.

The behavior is now that the mapping files are automatically referenced in development mode, but dropped in production.

This is probably my last big addition in the myfaces 4.0.0 jsf.js related codebase, unless something crawls up bugwise.

[volosied]

One item I found -- this code doesn't seem to handle jakarta.faces.Resource in ajax responses Such as <?xml version="1.0" encoding="UTF-8"?><partial-response id="j_id__v_0"><changes><update id="jakarta.faces.Resource"><![CDATA[<script src="/test-faces23-ajax-4466/jakarta.faces.resource/addedViaHead.js.xhtml?ln=spec1423"></script>]]></update>.

Feel free to test spec1423.xhtml

test-faces23-ajax.war.zip

Source code is from: https://github.com/jakartaee/faces/tree/4.0.1/tck/faces23/ajax

Here's our current code for the resource check: https://github.com/apache/myfaces/blob/89c747e85615e3f33265e664c8361789f38ea7db/api/src/main/javascript/META-INF/resources/myfaces/_impl/xhrCore/_AjaxResponse.js#L531-L535

[werpu]

Hi guys, I would be ready for merge, now... please complete the review and if you have objections please point them out for correction. If all goes well I can wrap everything up this week.

[werpu]

Ok just saw the comment, I will fix the issue. Thanks for the feedback and BBohman who pointed me towards it.

[werpu]

One item I found -- this code doesn't seem to handle jakarta.faces.Resource in ajax responses Such as <?xml version="1.0" encoding="UTF-8"?><partial-response id="j_id__v_0"><changes><update id="jakarta.faces.Resource"><![CDATA[<script src="/test-faces23-ajax-4466/jakarta.faces.resource/addedViaHead.js.xhtml?ln=spec1423"></script>]]></update>.

Feel free to test spec1423.xhtml

test-faces23-ajax.war.zip

Source code is from: https://github.com/jakartaee/faces/tree/4.0.1/tck/faces23/ajax

Here's our current code for the resource check:

https://github.com/apache/myfaces/blob/89c747e85615e3f33265e664c8361789f38ea7db/api/src/main/javascript/META-INF/resources/myfaces/_impl/xhrCore/_AjaxResponse.js#L531-L535

Hi I will have a look, thanks for reporting it.

Reedit: Spec section to this: If an update element is found in the response with the identifier jakarta.faces.Resource:

<update id="jakarta.faces.Resource"> <![CDATA[...]]> </update> append any element found in the CDATA contents which is absent in the document to the document's head section.

I already have added the unit tests on my github project to cover this, consider it fixed in the next few hours.

One item I found -- this code doesn't seem to handle jakarta.faces.Resource in ajax responses Such as <?xml version="1.0" encoding="UTF-8"?><partial-response id="j_id__v_0"><changes><update id="jakarta.faces.Resource"><![CDATA[<script src="/test-faces23-ajax-4466/jakarta.faces.resource/addedViaHead.js.xhtml?ln=spec1423"></script>]]></update>.

Feel free to test spec1423.xhtml

test-faces23-ajax.war.zip

Source code is from: https://github.com/jakartaee/faces/tree/4.0.1/tck/faces23/ajax

Here's our current code for the resource check:

https://github.com/apache/myfaces/blob/89c747e85615e3f33265e664c8361789f38ea7db/api/src/main/javascript/META-INF/resources/myfaces/_impl/xhrCore/_AjaxResponse.js#L531-L535

Hi I will have a look, thanks for reporting it.

Reedit: Spec section to this: If an update element is found in the response with the identifier jakarta.faces.Resource:

<update id="jakarta.faces.Resource"> <![CDATA[...]]> </update> append any element found in the CDATA contents which is absent in the document to the document's head section.

I already have added the unit tests on my github project to cover this, consider it fixed in the next few hours.

@volosied I have added you as reviewer, please retest as soon as it is fixed

[werpu]

I have fixed the reported resource processing, issue reported in https://github.com/apache/myfaces/pull/356#issuecomment-1299148181

and also documented under https://github.com/werpu/jsfs_js_ts/issues/25

I have added 3 tests, and also have covered one corner case where a resource theoretically (will never happen, but it would be possible from the protocol itself) could come in as embedded script or css. Either way unit tests are in place now which test for exactly the issue reported and which cover the spec and the additional possible side behavior. Implementation works on the unit tests. Please also test against the TCK!

[volosied]

@werpu Thanks for working on this!

I tested it out, and it's improved. However, I still see some odd behavior.

• The CSS stylesheets are applied (i.e background color changes), but the JS scripts aren't executed nor do I see network requests for them?

• The other thing I noticed is that resources are applied to the HEAD more than once ( which I don't think is intended per the TCK)

[werpu]

@werpu Thanks for working on this!

I tested it out, and it's improved. However, I still see some odd behavior.

• The CSS stylesheets are applied (i.e background color changes), but the JS scripts aren't executed nor do I see network requests for them?
• The other thing I noticed is that resources are applied to the HEAD more than once ( which I don't think is intended per the TCK)

I tested it out, and it's improved. However, I still see some odd behavior.

• The CSS stylesheets are applied (i.e background color changes), but the JS scripts aren't executed nor do I see network requests for them?
• The other thing I noticed is that resources are applied to the HEAD more than once ( which I don't think is intended per the TCK)

Ok I will do another round of testing... no resources should not be applied more than once... I have to check that and scripts should be executed. I will add additional tests and check where the problem is, thanks for testing. I will use your example now for testing (I thought the fix was easy enough to cover it with my unit tests only. Well sloppyness bites back instantly.

reedit: already found the issue for the not loading of resources, my fault. I did not trigger the explicit xhr loading code, browsers do not load automatically new code after the initial page load via the head appendix eval method. I did not write a testcase testing explicitely the loading because I assumed it would happen automativally.

As for the double includes, I have to check your example, wich I will do tomorrow. Atm i just append the resources as they come in, the spec just states whatever is in the cdata needs to be appended. So now if a resource is coming in twice, atm it is appended twice. If it is only coming in once then i have a problem in my code. Otherwise I have to check our old code how we handled that case. We probably simply can ignore resources already present in the page.

[volosied]

For the duplicates, I click the button twice which then add the script / link elements each time. I saw Mojarra had map to avoid this problem. Could we do the same?

[werpu]

For the duplicates, I click the button twice which then add the script / link elements each time. I saw Mojarra had map to avoid this problem. Could we do the same?

Yes definitely, another solution simply would be to check for elements with the same tags and src or hrefs. Given we use querySelectorall, the speed impact for doing this is neglectable.

I will add a fix and improved unit tests for this issue tomorrow, and I will also test the final code against your testcase, to make it watertight.

Not sure if a filter map really is the best way there because it ignores that a resource already might be on the dom once the first ajax call delivers a resource request, so you end up having a dom lookup to fix this. Then in a replace head or viewroot situation you have to clear the map anyway. So in the end you add additional bookkeeping and only gain a minor speed improvement in an area where speed is not critical anyway (how long des a querySelectorAll on the head take, probably not even measurable in nanoseconds).

[werpu]

Short update... Still working on the issue. While fixing this one I noticed that script src elements in Ajax calls were loaded asynchronously thus causing race conditions for mixed script src and embedded script tags. Apparently a bug which we also have in the old implementation, but never reported. So I had to fix this area as well. The solution was to change the way we eval script src="xxx" tags, i moved those also to the head appendix method which we already use for embedded scripts instead of running an xhr fetch request.

I have the fix now running and all my eval tests pass, but I broke one of my nonce tests along the way with the new code (the final integration test fails which tests for one of the nonce corner conditions)

So I will fix this one as well, and after that run the code against the linked war. But for now it is getting late, and I am calling it a day. Sorry for the delay. The merge probably can happen early next week.

[werpu]

Status update: Sorry it took longer, but given I discovered a race condition with a mix of script and script src= tag elements I had to overhaul the algorithm how we load script src= elements. We probably have the same issue in the old code. The code now works on my side, I also added unit tests for the reported cases of double include and src files not being loaded (some test code is now externalized into test fixtures which was before inline)

The last test pending is the one against the attached war, I will do that now before giving the general ok. But if you want to start testing feel free not to wait for me. If all goes well I then I will start the merge on monday.

[werpu]

Ok, thumbs up. The tests posted here now work for me as well. I noticed however that the tests themselves have a test which is not implementable.

Add resource to body the spec does not allow that ATM (javax.faces.Resource must be attached to the head)

results in:

a standard resource definition on the response protocol. atm. the resource response has no target attribute, so it is automatically attached to the head (not sure what the point of this test is)

Either way the tests now pass, please continue testing. I will monitor the pr, and if nothing else crawls up, will merge on monday.

[werpu]

Ok... I also got a positive report in from Tobago... so @volosied if you can give the ok from your side, I would be ready to merge.

[werpu]

Btw. theoretically we also could replace the codebase or add the next gen codebase to 2.3 (as second loading option) the codebase can work in both namespaces, via frontend shims)

[volosied]

@werpu

Wanted to give you a quick update. Most tests are passing, but I see a few issues

1. f:param doesn't to be properly sent. For example:

            <h:commandButton id="submitButton" value="Click here" action="#{elImplicitObjectBean.execute}">
                <f:param name="message" value="Hello World"/>
            </h:commandButton>

The code above sends the following in the form data: 0 "message,Hello+World".

0	"message,Hello+World"
form1_SUBMIT	"1"
jakarta.faces.ViewState	"N2RjNThlYTRlZjExNjJiODAwMDAwMDAz"
jakarta.faces.ClientWindow	"10tijvumt"
form1:_idcl	"form1:submitButton"

However, previously (working case), message "Hello+World" was sent.

form1_SUBMIT	"1"
jakarta.faces.ViewState	"ZjAyNTllMDgwODcxZTFlOTAwMDAwMDA0"
jakarta.faces.ClientWindow	"-uveyxftzi"
message	"Hello+World"
form1:_idcl	"form1:submitButton"

This causes our app to fail since ExternalContext's getRequestParameterMap() cannot find "message".

2. Clicking "Click Here" on ELImplicitObjectsViaCDI/index.xhtml open up a new tab, but this shouldn't happen. See apps and source.

3. Our Websocket Listener's onOpen call is called during the page loading, but it should only occur on the button click.

URL is: WebSocket/faces40/OpenCloseWebSocketTest.jsf

4. Related to the websocket issue above, pressing the onClose button (to close websocket channel) encounter this exception:

Uncaught TypeError: e.components[s].onclose is not a function
    onclose http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    onclose http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    bindCallbacks http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    open http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    open http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    open http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    init http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    init http://localhost:9080/WebSocket/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces:2
    <anonymous> http://localhost:9080/WebSocket/faces40/OpenCloseWebSocketTest.jsf:20

5. Our html unit library is encoutnering a script exception? I don't don't see this in the browser, so I think it's just our library. We'll try to see if it goes away with a version update. I validated the JavaScript syntax myself (compressed and uncompressed files) and I didn't find anything. However, could you also double check? Exception isn't clear where this "error" is.

com.gargoylesoftware.htmlunit.ScriptException: syntax error (http://localhost:8010/CDIManagedProperty/jakarta.faces.resource/faces.js.jsf?ln=jakarta.faces#2)

Source code can be found: https://github.com/OpenLiberty/open-liberty/tree/integration/dev/com.ibm.ws.jsf.2.3_fat/test-applications/ELImplicitObjectsViaCDI.war

https://github.com/OpenLiberty/open-liberty/tree/integration/dev/com.ibm.ws.jsf.2.3_fat/test-applications/WebSocket.war

But I've also attached the apps. Apps.zip

[volosied]

Another thing I just noticed: The ajax file submissions (single / multiple) doesn't seem to work.

https://github.com/jakartaee/faces/tree/master/tck/faces40/inputFile

[werpu]

Thanks for the reports, I will tackle every single one of those issues tomorrow. the f:param is clearly still an issue in the new form encoding code as for the rest I will have a deeper look. I have fixed an issue in this area a while ago, but as it seems, I might have missed a corner case there.

File submissions used to work, but I need to recheck them, i have moved them to the appropriate form javascript elements now from the old multi part mechanism, this might be a backend issue, but nevertheless I will check this as well.

Most if not all should be fixed tomorrow or the day after tomorrow.

Thanks for reporting the test results.

[werpu]

I have created an issue on my github side: https://github.com/werpu/jsfs_js_ts/issues/26

I will fix the bugs there first and will keep an update on the status. Once everything is fixed, it is merged back into the pull request. So if you want to keep track of things, you can follow there.

Short update: Could not reproduce the parameter issue (see linked issues comments, this could reference to an outdated faces.js codebase, I am in contact regarding this with @volosied ) I found the issue regarding the file uploads, and fixed it in my github codebase. Tackling now the push issue, which is weird, because thats the code I basically have not touched at all except for putting it into the new namespace. Might be a porting regression.

update: Regarding the push issue, there seems to have been a silent api change (or I have missed it in the spec change overview), apparently the Push Init has a breaking change in 4.0, you now must pass an onerror handler after onmessage in the init call. This change was done in 4.0 because 3.0 does not have it yet. This explains the auto connect issue we have had, because instead of the boolean flag autoConnect the parameter before that was passed as value and hence interpreted always as true. I have fixed this locally but cannot test it because I cannot open a socket connection in our impl. Getting a Socked refused despite having everything by the book (Sockets enabled in the web.xml, Server can run sockets, having the beans declared as Named and ApplicationScoped

@ApplicationScoped
@Named
public class WebsocketEndpoint implements Serializable {
    @Inject
    @Push(channel = "hello")
    PushContext hello;


    public String doSimplePush() {
        hello.send("Hello from Websocket endpoint");
        return null;
    }

}

produces following Error:

WebSocket connection to 'ws://localhost:8080/IntegrationJSTest/jakarta.faces.push/hello?07c148497ef95e29205f8ba55bd3044d' failed: 

I now have moved my integration test code from Weld, to Openwebbeans, same result. Not getting any connection despite Tomcat allowing websocket connections. The problem definitely is on our impl. I will isolate the problem and then send in a bug with a war showing the issue.

[werpu]

I for now have fixed the file upload issue and the push.init issue (silent api change), the f:param issue was not reproducible. I cannot test the push code atm because of a myfaces issue I face. I will isolate the problem in a self starting jar tomorrow and file a bugreport. The fixes are all comitted.

The script library issue very likely just is a test engine issue, I will investigate this tomorrow. Very likely we touch parts of the dom the library does not support, or the EcmaScript level is too high. (Atm es2015 is the compile target) We can compile to a lower level if needed, it just was not necessary because the browsers we support all are on es1015 level.

[werpu]

Hi I have isolated the problem and filed a bugreport: https://issues.apache.org/jira/browse/MYFACES-4496

If anyone wants to look into this, self running example for demonstrating it is linked to a github project I just opened!

[melloware]

cc @milansie as WebSockets was just refactored with this PR: https://github.com/apache/myfaces/pull/327

[werpu]

Cool thanks waiting for the merge then, after that I will check the issue again, and if fixed can close it.

[milansie]

hi, onerror was added here https://issues.apache.org/jira/browse/MYFACES-4415

@werpu you do not have **jakarta**.faces.ENABLE_WEBSOCKET_ENDPOINT enabled in your app. If you enable it, your demo will work..

[werpu]

hi, onerror was added here https://issues.apache.org/jira/browse/MYFACES-4415

@werpu you do not have **jakarta**.faces.ENABLE_WEBSOCKET_ENDPOINT enabled in your app. If you enable it, your demo will work..

Uuups yes, indeed, I was using the javax.namespace for the websocket web.xml init params, the error now is gone if I move over to the faces one. (stupid me) Thanks for pointing it out. I have closed the issue now as invalid.

My push test now passes... so for me push is fixed. The error reported indeed was a small api change from faces 3.0 to faces4.0 in jsf.push.init

@volosied, can you retry your tests with the current codebase? Everything is in myfaces under this pull request. I will have another look at your parameter problem, but my personal guess is given I now have this covered by an integration test and could not reproduce it that this was a problem that an old codebase was overriding my implementation or you had an old codebase:

  <ui:define name="content">

        <components:jasmineTest testRunner="test25-f-param.js"/>

        <h:form id="form1" prependId="false">
            <!-- needed for the test trigger -->
            <h:commandLink id="submitButton" value="Click here" action="#{myBean2.execute}">
                <f:ajax execute="@this" render="output" />
                <f:param name="hello2" value="Hello from f:param"/>
                <f:param name="Hello World" value="Hello from f:param2"/>
            </h:commandLink>

            <h:panelGroup id="results">
                <h:outputText id="output" value="#{myBean2.helloWorld}"></h:outputText>
            </h:panelGroup>
        </h:form>
    </ui:define>

 public String execute() {
        Map<String,String> params =
                FacesContext.getCurrentInstance().getExternalContext().getRequestParameterMap();
        this.helloWorld = params.get("hello2");
        return null;
    }

I will check the html unit problem you face tomorrow, but I guess it is simply a deficit in the driver used by HTMLUnit. As you said the browsers do not expose the problem. I am running my teste here with puppeteer and jasmine aka... the tests run in a semi recent embedded chromium engine. Given we have our cut off with Chromium/Safari/Firefox and maybe (Trident) The used HTMLUnit driver might not cover our baseline entirely. The fix might simply be to move HTMLUnit to chrome or whatever semi recent engine is supported but not to run it on a custom driver interpreting a browser.

[werpu]

Found the issue, apparently html unit bundles Mozilla Rhino and the bundled version or any version (still not fully done with my research on that yet) does not support Ecmascript 2015 syntax. After compiling the files down to es5 I get a more meaningful error in my internal testing framework that the keyword class (explicit classes were introduced in es2015) is not allowed. I checked on Mozillas site and found this: https://mozilla.github.io/rhino/compat/engines.html

There are two possible fixes to move forward a) We can return to an ES5 compile target, which is rather pointless given the apis we use, and might run into compatibility issues with html unit anyway (we use FormData elements XhrObject, querySelectorAll etc... which are APIs which might not exist on ES5 level browser implementations) But nevertheless it is worth a shot, but my hopes are somewhat limited. In my opinion HTML Unit is outdated, unless they fix their Rhino problem. There are alternatives.

b) You guys can have a look to get the unit tests out of this ES5 level maybe move the tests to Selenium where you can use different drivers or move to a client side testing framework. I know this is a little but much to ask. But as it seems HTMLUnit for now as testrunner seems to be a dead end as long as it uses Rhino and/or Rhino does not move forward.

A viable option for a testing with a more recent engine would be to use Selenium Webdrivers with headless chrome (you need to have a chrome instance preinstalled in the testing environment though)

https://gist.github.com/werpu/5f212b8c6ee8864cbfdceec8bbff226b

Gives you the idea how to pull this off.

And the configuration for it would be: https://gist.github.com/werpu/94f4a74007ac6efae9561e111e216b42

So you get the idea...

Basically the same as html unit but with chrome as engine (headless) or firefox etc... in non headless mode.

This then can be combined by the usual means into tests. Just to sum it up outside of having a browser engine in pure java and maybe having the need to check for explicity es5 compatibility for legacy reasons, there is no advantage in using HTMLUnit, you still can use it as a webdriver in such a setup, but then you will get all downsides the same way we did by calling it directly.

[melloware]

My two cents not sure if it helps or not but...

1. I don't want to go back to ES5 compatibility just to make the HTMLUnit tests work.
2. Upgrading from HtmlUnit to Selenium would be a MUCH bigger change.