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

make all component SSR compatible #9187

Merged
merged 18 commits into from
Aug 31, 2024
Merged

make all component SSR compatible #9187

merged 18 commits into from
Aug 31, 2024

Conversation

pngwn
Copy link
Member

@pngwn pngwn commented Aug 23, 2024

description

closes #9111 closes #9112

Makes all component SSR compatible.

  • Removes any bare references to browser only APIs, or hide them behind on Mount hooks or actions we know can only occur when we are in the browser and fully loaded
  • Stop using location.origin and instead use the root url value that we have access to.
  • Added some sensible defaults to avoid things jumping around too much when hydrated in the browser
  • Use empty value for certain components until we are in the browser, things like native plots or model 3d don't make much sense in a server context. We can improve this later if needed.
  • Added an SSR-able test app with some example values.
  • Switch to an isomophic (server + browser compatible) version of dompurify.

to test

pnpm i
cd js/component-test
pnpm dev

While running the app. I would recommend doping the following to see how things behave:

  • Visit every link at the top and hard refresh the browser (with dev tools closed to you get cached fonts). This is Server Side Rendering + Client Side Rendering (hydration).
  • Try with JS disabled:
    • Go into chrome dev tools
    • Click the cog in the top right
    • scroll down to "Debugger"
    • Click "Disable JavaScript"
    • Keep dev tools open and revisit all links + refresh, this is SSR only.

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 23, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-pypi-previews.s3.amazonaws.com/c440b0d45a2a0501dc54f02e64330e966964c848/gradio-4.42.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@c440b0d45a2a0501dc54f02e64330e966964c848#subdirectory=client/python"

Install Gradio JS Client from this PR

npm install https://gradio-npm-previews.s3.amazonaws.com/c440b0d45a2a0501dc54f02e64330e966964c848/gradio-client-1.5.1.tgz

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 23, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/annotatedimage patch
@gradio/audio patch
@gradio/chatbot patch
@gradio/code patch
@gradio/dataframe patch
@gradio/dataset patch
@gradio/dropdown patch
@gradio/file patch
@gradio/imageeditor patch
@gradio/markdown patch
@gradio/model3d patch
@gradio/nativeplot patch
@gradio/paramviewer patch
@gradio/utils patch
@gradio/wasm patch
@self/component-test patch
gradio patch
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

make all component SSR compatible

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@pngwn pngwn added the v: patch A change that requires a patch release label Aug 23, 2024
@@ -84,13 +84,13 @@

let editor_box: EditorContext["editor_box"] = writable({
parent_width: 0,
parent_height: 0,
parent_height: 400,
Copy link
Member Author

Choose a reason for hiding this comment

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

use provided height value if it is passed

@@ -31,6 +31,7 @@
"@types/prismjs": "1.26.4",
"dompurify": "^3.0.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

remove

Comment on lines 69 to 77
DOMPurify.addHook("afterSanitizeAttributes", function (node) {
if ("target" in node) {
if (is_external_url(node.getAttribute("href"))) {
node.setAttribute("target", "_blank");
node.setAttribute("rel", "noopener noreferrer");
}
}
});
const is_browser = typeof window !== "undefined";
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this need to be moved?

Do we need the browser var?

@@ -64,9 +64,14 @@
is_downloading = false;
});
}

let should_proxy = false;
onMount(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove?

@hannahblair
Copy link
Collaborator

Thanks for going through the changes! Have done a test and can't see any issues. Once those comments are sorted lgtm :)

@dawoodkhan82
Copy link
Collaborator

@pngwn was reviewing the components yesterday and it looked good to me. But now I'm getting "SSR Module" errors when running the demo. Could just be me tho.

@abidlabs
Copy link
Member

@pngwn was reviewing the components yesterday and it looked good to me. But now I'm getting "SSR Module" errors when running the demo. Could just be me tho.

Locally or on the Deployed PR Space @dawoodkhan82?

@dawoodkhan82
Copy link
Collaborator

@abidlabs locally

@abidlabs
Copy link
Member

I can't repro that @dawoodkhan82. I checked out this branch, rebuilt the frontend, and ran some demos (like kitchen_sink) and they worked fine for me. I do have Node v18 installed in case that makes any difference

@dawoodkhan82
Copy link
Collaborator

dawoodkhan82 commented Aug 29, 2024

@abidlabs did you try with disabled JS from the dev tools?

The components-test demo looks good to me, but once I disable JS (SSR only) then I get 500 internal server errors when clicking through the components.

@abidlabs
Copy link
Member

abidlabs commented Aug 29, 2024

Ah yeah sorry missed those testing instructions. I'm seeing 500 errors as well. The web app looks like this:

image

Partial stack trace:

1:36:25 PM [vite] Error when evaluating SSR module /@fs/Users/abidlabs/dev/gradio-repos/gradio/js/atoms/src/ShareButton.svelte: failed to import "@gradio/utils"
|- Error: Failed to resolve entry for package "@gradio/utils". The package may have incorrect main/module/exports specified in its package.json.
    at packageEntryFailure (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48264:17)
    at resolvePackageEntry (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48261:5)
    at tryNodeResolve (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48039:20)
    at nodeImport (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:55087:26)
    at ssrImport (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:54998:30)
    at eval (/Users/abidlabs/dev/gradio-repos/gradio/js/atoms/src/ShareButton.svelte:7:37)
    at async instantiateModule (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:55058:9)

1:36:25 PM [vite] Error when evaluating SSR module /@fs/Users/abidlabs/dev/gradio-repos/gradio/js/atoms/src/index.ts: failed to import "/@fs/Users/abidlabs/dev/gradio-repos/gradio/js/atoms/src/ShareButton.svelte"
|- Error: Failed to resolve entry for package "@gradio/utils". The package may have incorrect main/module/exports specified in its package.json.
    at packageEntryFailure (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48264:17)
    at resolvePackageEntry (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48261:5)
    at tryNodeResolve (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48039:20)
    at nodeImport (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:55087:26)
    at ssrImport (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:54998:30)
    at eval (/Users/abidlabs/dev/gradio-repos/gradio/js/atoms/src/ShareButton.svelte:7:37)
    at async instantiateModule (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:55058:9)

1:36:25 PM [vite] Error when evaluating SSR module /@fs/Users/abidlabs/dev/gradio-repos/gradio/js/multimodaltextbox/shared/MultimodalTextbox.svelte: failed to import "/@fs/Users/abidlabs/dev/gradio-repos/gradio/js/atoms/src/index.ts"
|- Error: Failed to resolve entry for package "@gradio/utils". The package may have incorrect main/module/exports specified in its package.json.
    at packageEntryFailure (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48264:17)
    at resolvePackageEntry (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48261:5)
    at tryNodeResolve (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:48039:20)
    at nodeImport (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:55087:26)
    at ssrImport (file:///Users/abidlabs/dev/gradio-repos/gradio/node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected][email protected][email protected][email protected]_/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:54998:30)

@pngwn
Copy link
Member Author

pngwn commented Aug 29, 2024

I'll take a look at this. Seems to just be the utils package that is causing issues.

@pngwn
Copy link
Member Author

pngwn commented Aug 30, 2024

@abidlabs @dawoodkhan82 I left something out of my instructions which doesn't affect other packages but apparently does affect the utils package for some reason.

You need to run pnpm packages from the root first. That should fix the issue you were seeing.

@abidlabs
Copy link
Member

Ok working for me now, but noticing a few small things. Let me comment as I find them:

(1) i18n strings are not rendering correctly, e.g.

image image

(2) Audio player is not coloring the rendered audio appropriately. You can see here that its hiding the played audio instead of coloring it orange:

Screen.Recording.2024-08-30.at.9.50.23.AM.mov

@abidlabs
Copy link
Member

(3) Nothing is shown for gr.Dataset -- I assume that's just because the value is set to null, but flagging just in case:

image

(4) Uploading audio isn't working. I noticed the same for the Interactive Gallery. I think all Upload is broken.

Screen.Recording.2024-08-30.at.9.53.51.AM.mov

@abidlabs
Copy link
Member

(5) Issue #4 is preventing me from being able to fully test gr.ImageEditor, but it looks like the bottom of the component is cut off:

image

and also when I click on the draw tool, its very laggy and colors the canvas at the wrong spot when clicked

@abidlabs
Copy link
Member

(6) Clicking on the Native Plots tabs causes the whole app to crash, with this appearing in the console:

image

Those are the issues I noticed, happy to give this another review afterwards as needed!

@pngwn
Copy link
Member Author

pngwn commented Aug 30, 2024

Okay, almost all of those are expected. The main purpose of this test app is to test the initial rendering (i.e. will the component initialise on the server) not the full functionality. This test app lacks the business logic of the gradio app to do that properly.

  1. there is no i18n just an identity fn, so it is returning whatever is passed in, this is expected.
  2. I have broken audio in several ways, working on a fix.
  3. This is correct, the way we load examples is weird and the test app cannot do it.
  4. Uploading doesn't work in the test app as there is no server, only a frontend, so we have nowhere to upload files to.
  5. I broke the image height, also fixing.
  6. This shouldn't happen, native plots should render anything in SSR.

So 1, 3, 4 are expected. I'll take care of the others.

For 1, 3, 4 specifically, as long as they work in gradio proper, then they are working as expected.

@pngwn
Copy link
Member Author

pngwn commented Aug 30, 2024

Fixed everything but can't reproduce #4.

Screen.Recording.2024-08-31.at.02.31.18.mov

@abidlabs
Copy link
Member

Fixed everything but can't reproduce #4.

Can't repro it anymore either!

@abidlabs
Copy link
Member

Ok went through and tested everything with and without JS, everything works great @pngwn with one exception: native plots don't work if you start with a different component and then switch to the native plots tab. If you hard refresh, then they work. Recording:

Screen.Recording.2024-08-30.at.12.06.44.PM.mov

@pngwn
Copy link
Member Author

pngwn commented Aug 30, 2024

Oh interesting.

@pngwn
Copy link
Member Author

pngwn commented Aug 31, 2024

Okay fixed!

Gonna merge this in so I can continue but feel free to make any additional comments and i'll fix in a follow up!

@pngwn pngwn enabled auto-merge (squash) August 31, 2024 06:08
@pngwn pngwn merged commit 5bf00b7 into main Aug 31, 2024
21 of 22 checks passed
@pngwn pngwn deleted the ssr-components branch August 31, 2024 06:19
This was referenced Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v: patch A change that requires a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make all incompatible components SSR compatible Review components for SSR compatibility
5 participants