TheCoder4eu / BootsFaces-OSP

BootsFaces - Open Source Project
Apache License 2.0
246 stars 102 forks source link

b:dataTable causes JavaScript errors in Liferay 7.0 due to CDN resource #853

Closed stiemannkj1 closed 6 years ago

stiemannkj1 commented 7 years ago

Liferay 7.0 exposes an AMD loader and certain JS resources do no behave well when an AMD loader is present due to anonymous modules. In order to work around these errors, the Liferay Faces Bridge filters the content of these resources slightly to disable their AMD loader features. datatables.min.js is one of the resources that we must filter. With BootsFaces v1.1.2 things worked fine, but with v1.1.3 datatables.min.js was moved to using a CDN. Since datatables.min.js is no longer present on the server, the Liferay Faces custom ResourceHandler can no longer filter the datatables.min.js resource to make it compatible with Liferay 7.0's frontend JS.

In order to fix this, datatables.min.js should be included in the BootsFaces jar (or a ResLib jar) as it was in v1.1.2 and an option should probably be included to enable loading the resource from a CDN.

chsreedhar commented 7 years ago

@stephanrauh, include datatables.min.js and datatables.min.css in to jar, otherwise <b:dataTable /> not going to work without internet connection.

stephanrauh commented 7 years ago

@chsreedhar We've extracted the file because it adds a huge amount of memory to the core library. However, we can split the BootsFaces.jar into two jars: one containing the core library, and an optional jar containing the datatable files. If the optional library is not there, we use the CDN approach. How do you like this idea?

chsreedhar commented 7 years ago

@stephanrauh nice idea.

paulohva commented 6 years ago

@stephanrauh Is this enhancement planned to be in upcoming releases? I can't update my version of Bootsfaces without a solution for this.

stephanrauh commented 6 years ago

@TheCoder4eu What's your opinion?

NMagarreiro commented 6 years ago

@stephanrauh and @TheCoder4eu, there are any news about this issue? Our client machines does not have internet connection, so we need this issue resolved in order to upgrade BootsFaces version.

NMagarreiro commented 6 years ago

Hi @stephanrauh and @TheCoder4eu, I checked that this problem still happens on BootsFaces 1.3.0. Is this correction planned to a future release?

Thanks.

stiemannkj1 commented 6 years ago

@stephanrauh, @TheCoder4eu, could you make the URL for the resource configurable via init-param so that people could provide their own local version if they want?

stephanrauh commented 6 years ago

@stiemannkj1 @chsreedhar @NMagarreiro @paulohva First of all, sorry for forgetting this ticket. Somehow, it got buried under a pile of new tickets.

But maybe it's good we didn't implement the solution in a hurry. Kyle, I like your idea better than my original idea. Maybe you're familiar with our approach to using jQuery? I'd like to implement the same idea for the datatable:

How do you like the idea?

stiemannkj1 commented 6 years ago

@stephanrauh, sounds good to me. Perhaps you could include an example or documentation showing how to use this feature as part of the ticket?

stephanrauh commented 6 years ago

@stiemannkj1 Yes, sure - but give me some time :). I've pushed a tentative implementation to GitHub. It passed my first few tests, but even so, take it with a grain of salt. If you're ready to dive into the sourcecode (it's a fairly small change), follow the link above.

stiemannkj1 commented 6 years ago

@stephanrauh, looks good :) Minor tweaks:

You may be able to skip the looping check if get_datatable_from_cdn == false. You may be able to abort the loop (break) when loadDatatables is set to false.

stephanrauh commented 6 years ago

@stiemannkj1 @chsreedhar @paulohva @NMagarreiro I can't decided how to name the context parameter. Is it more intuitive to call it net.bootsfaces.get_datatable_from_cdn? Or should we use the plural form, because the JS widget is called DataTables.net?

IMHO we should prefer the singular form because that's the name BootsFaces uses (<b:dataTable>). But it feels a bit odd to ignore the naming pattern of the underlying JS widget, especially taking into account how much work and passion they've invested in their pet framework. What's your opinion?

stephanrauh commented 6 years ago

Current state of the art: there's some documentation, but the live demo is still being developed. What's already there should appear soon at https://showcase.bootsfaces.net/layout/resourcemanagement.jsf.

chsreedhar commented 6 years ago

@stephanrauh use context parameter net.bootsfaces.get_datatable_from_cdn and go ahead with your proposal above.

stephanrauh commented 6 years ago

I've finished the first demo at https://staging.bootsfaces.net/Showcase/layout/resourcemanagement.jsf.

Currently, the demo only demonstrates how to use the web.xml parameter. The other detection mechanisms still need a demo in the showcase.