liferay / liferay-js-toolkit

GNU Lesser General Public License v3.0
52 stars 41 forks source link

Pure JS portlets fail to instantiate when they have complex portlet preferences associated #364

Closed mfreeman-xtivia closed 5 years ago

mfreeman-xtivia commented 5 years ago

React applications generated using the Liferay-JS yeoman generator do not appear to work at all when used as fragments on the new Liferay DXP 7.2 "content" pages.

The generated portlet works fine on a "widget" page but when placed on a content page simply displays as blank.

Reproduced this on a clean, fresh copy of DXP 7.2. Generated an OOB React portlet and deployed to the fresh DXP. Portlet works fine on widget page but not content page. Other non-JS portlets (e.g. blogs) work just fine as fragments on the content page.

mwilliams2014 commented 5 years ago

Hey @mfreeman-xtivia , By default, only certain portlets can be embedded in a fragment. For custom portlets, you must configure it to make it embeddable. Our docs go into more detail on this: https://portal.liferay.dev/docs/7-2/reference/-/knowledge_base/r/fragment-specific-tags#including-widgets-within-a-fragment In the case of portlets generated with the Liferay-JS generator, you must add the portlet property com.liferay.fragment.entry.processor.portlet.alias in the portlet entry of the package.json as described in our docs here: https://portal.liferay.dev/docs/7-2/frameworks/-/knowledge_base/f/configuring-portlet-properties-for-your-widget Have you tried this? If not, please try and let me know if it works for you. Thanks!

mfreeman-xtivia commented 5 years ago

Understood. I will give that a try. But my first reaction would that even if that works its pretty subtle and no doubt many will fly right past it and bump into the same issue i did. So it might be worth thinking about how to better illuminate that requirement....

And as a last question, I am assuming that in the case of using the drag-n-drop content page builder, it doesnt matter what value i set that alias to, as long as its unique?

mwilliams2014 commented 5 years ago

Noted. FYI, I just updated the docs link to point to our 7.2 reference article on the subject. I'll see how we can update the docs to make the workflow more obvious. Thanks! As long as it's the same app-name you define in the portal property, it shouldn't matter.

mfreeman-xtivia commented 5 years ago

So i tried your solution and set that property to an arbitrary value in my package.json. Then after a redeploy i again tried to drag-n-drop my LJS portlet as a widget onto a content page (see screenshot below). And it makes no difference. The area is still blank. Note that this same drag-n-drop works just fine for conventional portlets like blogs etc

image

mwilliams2014 commented 5 years ago

Hmmm...And you added the <lfr-widget-app-name></lfr-widget-app-name> tags to the fragment (replacing the app-name with the arbitrary name you chose), I assume?

mfreeman-xtivia commented 5 years ago

We are talking past one another i think.

I am not using the fragment editor/designer. I went to the content page editor, then clicked the "Section Builder" icon on the far right. Dragged a 2-column empty section template to the page. Then the 3rd icon, "Widgets: on the far right under Section Builder. You see a list of categorized widgets there just like you do on a normal page.

Dragging a conventional widget e.g Blogs into your newly created empty section works fine. A JS generated portlet does not.

So you will note that this is NOT an attempt to create a fragment with JS/CSS/HTML editor where i would have access to the tags you refer to. This is ALL drag and drop

mwilliams2014 commented 5 years ago

Oh, I apologize. I misunderstood your issue. Hmmm...in that case, I would expect the widget would show as you said. Someone else will have to chime in with a solution. As they are in Spain, it will take some time. Sorry again for the misunderstanding.

jbalsas commented 5 years ago

Hey @p2kmgcl, do you think you could take a look here, see what we might be missing? Can you tell us if this is on your side or on ours?

p2kmgcl commented 5 years ago

Hey @mfreeman-xtivia @mwilliams2014 from 7.2, you don't need to include widgets inside a fragment to add them, so @mfreeman-xtivia is right, you only need to drop the widget into the page and it should be working.

Also from 7.2 any widget should be working, so let me check with the team if there is any special requirement that needs to be fulfilled to make it work.

Edit: There are no extra requirements, so it should work. I am going to try to reproduce this error.

Edit 2: I got this error in the browser console

SyntaxError: Unexpected token p in JSON at position 22
    at JSON.parse (<anonymous>)
    at <anonymous>:18:30
    at loader.js:379
    at loader.js:343

It looks like some file is not being transpiled. Continuing...

Edit 3: It does work on a widget page, so something wrong is going on with content pages.

Edit 4: There is something wrong when instantiating a widget with configuration, this is the generated code from a content page, which has a bad JSON:

initializer(
                    {
                        configuration: {
                            portletInstance: JSON.parse('{"portletSetupCss":"{\"portletData\":{\"useCustomTitle\":false,\"titles\":{},\"portletDecoratorId\":\"\"},\"advancedData\":{\"customCSSClassName\":\" portlet-barebone\"}}"}'),
                            system: JSON.parse('{}')
                        },
                        contextPath: '/o/react-widget',
                        portletElementId: 'js-portlet-_react_widget_INSTANCE_zfjfF3zOTsPk_',
                        portletNamespace: '_react_widget_INSTANCE_zfjfF3zOTsPk_'
                    });
jkappler commented 5 years ago

Hey all, this is also reproducible in widgets pages too. This is how you can achieve this:

  1. Add the portlet to a widget page
  2. In the portlet topper, go to Look and Feel Configuration
  3. Change to use custom title and provide it, or change the application decorator
  4. Save the config

The content of the portlet won't be shown after these steps. So it's most likely the look and feel configuration that's failing (as seen in portletInstance configuration that @p2kmgcl provided)

Regards

izaera commented 5 years ago

Apparently, when the portlet preferences of the portlet contain complex values, the generated JSON is corrupted. This is a server error, so I'm moving it to an LPS :point_right: https://issues.liferay.com/browse/LPS-98350

mfreeman-xtivia commented 5 years ago

@izaera can you please tell me more by what you mean as a "complex value"?

This would help me to work around it in the meantime

izaera commented 5 years ago

@mfreeman-xtivia To be honest I have no idea. I mean, it looks like the extender is passing all portlet preferences to the portlet's initializer, as can be seen here:

{"portletSetupCss":"{\"portletData\":{\"useCustomTitle\":false,\ ...

Those values are stored in portlet preferences by Liferay but the extender is mistakenly trying to serialize them and pass them to the portlet as configuration values.

I don't know why the generated JSON is incorrect because it shouldn't, as I'm using a generic JSON serializer in the server (see https://github.com/liferay/liferay-portal/blob/master/modules/apps/frontend-js/frontend-js-portlet-extender/src/main/java/com/liferay/frontend/js/portlet/extender/internal/portlet/JSPortlet.java#L144) but for some reason it fails.

However, no matter why it fails, those properties shouldn't be passed to the portlet as they are not configuration so I'm gonna fix that in first place.

Regarding how you can workaround it, I don't see how because it looks like the fragments infrastructure is adding those portlet preferences so there's nothing you may do, I guess :-(. Am I right @p2kmgcl ?

p2kmgcl commented 5 years ago

Sadly I think @izaera is right. If you could just use a simpler configuration (like just adding a plain object with strings or some) it should be working. But this is the default one, should it may fail everytime.

Maybe you can try in to embed this widget inside a fragment as @mwilliams2014 suggested and then, when the issue is fixed, add the widget directly to the content page. You just have to change a small config to do that https://portal.liferay.dev/docs/7-1/tutorials/-/knowledge_base/t/fragment-specific-tags#embedding-your-widget-in-a-fragment

mfreeman-xtivia commented 5 years ago

@p2kmgcl @izaera i guess that what i am still struggling with here. What is a "simple configuration" JSON that would you expect to work and a "complex configuration" that you would expect to fail?

izaera commented 5 years ago

@mfreeman-xtivia I used "complex" as a placeholder for "I don't know yet" :-(.

Until I get to fix the LPS I don't know why some portlet preferences fail and others don't :man_shrugging:

As I said I'm using a generic JSON serializer that should serialize everything correctly. Maybe it could fail to serialize something, but not generate an incorrect JSON for sure.

Let me find some time to analyze and start fixing the LPS and I may find a workaround while in the process. If I do I'll post it here.

Sorry.

izaera commented 5 years ago

There's a strange thing, though...

If you paste the code @p2kmgcl wrote above in a JS console:

JSON.parse('{"portletSetupCss":"{\"portletData\":{\"useCustomTitle\":false,\"titles\":{},\"portletDecoratorId\":\"\"},\"advancedData\":{\"customCSSClassName\":\" portlet-barebone\"}}"}')

it fails with an exception (that's why the portlet fails).

But if you copy the string:

{"portletSetupCss":"{\"portletData\":{\"useCustomTitle\":false,\"titles\":{},\"portletDecoratorId\":\"\"},\"advancedData\":{\"customCSSClassName\":\" portlet-barebone\"}}"}

And validate it here -> https://jsonlint.com/, for example, it says the JSON is valid :thinking: .

izaera commented 5 years ago

However, as I said, the portletSetupCss should never arrive to the portlet as it is not configuration but extraneous portlet preferences, so I won't dedicate time to investigate this oddity. I will just avoid passing portletSetupCss (or any other portlet pref that is not configuration) and everything will work fine after that.

izaera commented 5 years ago

@mfreeman-xtivia This has been merged into the portal codebase (see https://issues.liferay.com/browse/LPS-98350). The fix should arrive in the next fixpack.

rdai10 commented 5 years ago

Looks like the fix is released in fixpack 1!