Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix URL path for output renderer scripts #12976

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Oct 4, 2023

What it does

Fixes #12966
Fixes an issue with notebook api downloading output renderer scripts from base path instead of deployed path

How to test

Easiest for me was using nginx.

  1. run Theia locally
  2. Add following config to the server configuration of nginx
location /theia/ {
            rewrite ^/theia(/.*)$ $1 break;
            proxy_pass http://localhost:3000/;
            proxy_http_version 1.1;
            proxy_set_header Host $host;
        }
  1. open theia on http://localhost/theia
  2. open a notebook, execute a cell and see the output renderer works.

Follow-ups

Review checklist

Reminder for reviewers

@msujew msujew changed the title fixed path for downloading output renderer scripts from Fix URL path for output renderer scripts Oct 4, 2023
@msujew msujew added the notebook issues related to notebooks label Oct 4, 2023
@bvenreply
Copy link
Contributor

Hello @jonah-iden, we tested this solution but it's not working for us, we're getting this error now:

image

It seems the browser import function is being finicky about relative module paths. Prefixing a ./ seems to fix it:

image

@jonah-iden
Copy link
Contributor Author

jonah-iden commented Oct 11, 2023

Hi, sorry for the late reply.
My solution worked for me and was also used in other parts of theia. Maybe there is a bigger problem in theia then and i did not test thoroughly enough. I'll try to take a look later today.

@bvenreply
Copy link
Contributor

Hi, sorry for the late reply. My solution worked for me and was also used in other parts of theia. Maybe there is a bigger problem in theia then and i did not test thoroughly enough. I'll try to take a look later today.

No problem. Thank you @jonah-iden. Btw another possibility to consider is that I messed something up on my end...

@jonah-iden
Copy link
Contributor Author

@bvenreply, i was able to reproduce your error and improved my previous solution. For me this works great. could you test it again if you find the time?

@bvenreply
Copy link
Contributor

@jonah-iden I tested the new changes, I confirm it's working. Thanks!

@JonasHelming
Copy link
Contributor

@msujew We discussed yesterday that your approval is enough for this one to be merged

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, works well. I'm not 100% sure this is the best approach to this, but it works and we can easily change it in case there are any issues.

@jonah-iden jonah-iden merged commit c625ba1 into master Oct 12, 2023
13 checks passed
@msujew msujew deleted the jiden/notebook-fix-output-renderer-path branch October 12, 2023 10:23
@github-actions github-actions bot added this to the 1.43.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Notebook renderer basepath should be relative
4 participants