holoviz / panel

Panel: The powerful data exploration & web app framework for Python
https://panel.holoviz.org
BSD 3-Clause "New" or "Revised" License
4.61k stars 500 forks source link

Code Editor extension code clashing with external widgets code #6693

Open singharpit94 opened 5 months ago

singharpit94 commented 5 months ago

I am using panel 1.3.8 and not able to use any react based ipywidgets (via ipywidgets-bokeh ) which makes use of window.require and window.define method along with codeeditor extension.

The primary reason for that are pre-requirejs and post-requirejs files where we are overriding the default require and define methods with custom ones.

When we override, the context of both require and define gets affected and we are not able to use methods attach to require context such as require.config. Even if we attempt to modify our usage with the new variables (_require_requirejs or _define_requirejs) which are being assigned the original values, the resolution of the required modules are not correct and I bump into several new issues ,one of which says image

In my local setup, when I comment these overrides my widgets always load successfully though there are some issues with loading the ace editor.

Unfortunately our internal setup is complex and sharing a reproducer is not straight forward but in general I believe we should refrain from overriding these global methods which have associated context variables to avoid usages of them in other libraries.

Can you please check this and suggest way ahead?

ndmlny-qs commented 4 months ago

It looks like pre_require.js and/or post_require.js is used in the Python code here https://github.com/holoviz/panel/blob/bfbba8e29facd5b26fd499192e41c46e4e3fbe6f/panel/io/resources.py#L718-L727.

From the logic there, there might be a name collision occurring since the code looks for the substring ace within all the JavaScript files if you have require.js in your app. I would suggest inspecting the js_files list and see if the substring ace is found in your requirements. If it is, then the next steps would be to follow the stack to see if it makes sense to search for a more specific substring than ace.

singharpit94 commented 4 months ago

Hi @ndmlny-qs , I actually did not follow if the above changes are intended to be tried at the application/extension layer or in the panel code itself. Seems like to be in panel code but correct me if I am wrong.

ndmlny-qs commented 4 months ago

I do not think we need to make changes just yet, as I would like to see if the check happening on lines 723 and 726 above are picking up JS files in your app that it was not intended to.

I would like to see if when you put a breakpoint below line 722 above that you get True back with the any('ace' in jsf for jsf in js_files) check. If this is the case, and you do not have the ace editor as a requirement in your app, then we need to update the check since we are getting a false positive back from it.

singharpit94 commented 4 months ago

Correct me if I am wrong but I think I will have the ace requirement in my app if I am using the 'codeeditor' extension. Below is an example code

import panel as pn

pn.extension('codeeditor')

def simple():

    py_code = "import sys"
    editor = pn.widgets.CodeEditor(value=py_code, sizing_mode='stretch_width', language='python', height=300)

    # Create a button
    button = pn.widgets.Button(name='Dummy Button', button_type='primary')

    # Return the layout
    return pn.Row(react_widget, button, editor)

simple().servable()

I actually tried few more things and found that if I avoid popping out the requirejs files from this line, it actually loads my react widget and code editor fine, though I will need to test it more to cover all cases.

Any thoughts ?

ndmlny-qs commented 4 months ago

Looks like you are correct. Putting a breakpoint between line 720 and 721 from above

https://github.com/holoviz/panel/blob/bfbba8e29facd5b26fd499192e41c46e4e3fbe6f/panel/io/resources.py#L718-L727

and inspecting the js_files object for the substring ace does return results using a modified MWE (below). However, I do not see the require.js package as your MWE does not include it.

The comment on line 717 indicates that the logic should load require.js "last". In fact, what it does is remove require.js from the js_files object, loads pre_require.js adds require.js back and then adds post_require.js to the js_files object. I know this is the case as I injected require.js as the first string in the js_files list, and then ran it through the logic to end up with require.js sandwiched between pre_require.js and post_require.js as the last 3 objects in js_files, as was intended.

When I inspect the pre|post_require.js files in the browser I see that the window object for window.require returns the expected result of switching between the ace editor and require.js. I don't think there is anything wrong or unexpected at this point.

I can dive deeper if you create a MWE that includes a react component that also shows the error you are getting. I did inject react to the js_files object and used the ReactTemplate as an example for myself, and the modified MWE worked just fine for me.

@philippjfr I inspected the history around adding pre_require.js and post_require.js, but I am not clear as to why it was necessary. If you have a few minutes to explain it here I'd appreciate it, but it's not necessary.


MWE

import panel as pn
pn.extension("codeeditor")

def simple():
    py_code = "import sys"
    editor = pn.widgets.CodeEditor(
        value=py_code,
        sizing_mode="stretch_width",
        language="python",
        height=300,
    )
    button = pn.widgets.Button(name="Dummy Button", button_type="primary")
    return pn.Row(button, editor)

simple().servable()

Modified MWE

import panel as pn
pn.extension("codeeditor")

def simple():
    py_code = "import sys"
    editor = pn.widgets.CodeEditor(
        value=py_code,
        sizing_mode="stretch_width",
        language="python",
        height=300,
    )
    button = pn.widgets.Button(name="Dummy Button", button_type="primary")
    return pn.Row(button, editor)

template = pn.template.ReactTemplate(title="ReactTemplate", sidebar=[])
template.main[:3, :3] = simple()
template.servable()