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

feat: Add headers for wasm support #3260

Merged
merged 3 commits into from
Dec 7, 2023
Merged

feat: Add headers for wasm support #3260

merged 3 commits into from
Dec 7, 2023

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 26, 2023

  • Figure out actually required headers

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr marked this pull request as ready for review December 6, 2023 15:11
@juliusknorr juliusknorr added 3. to review Ready to be reviewed enhancement New feature or request labels Dec 6, 2023
Copy link
Contributor

@eszkadev eszkadev left a comment

Choose a reason for hiding this comment

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

Tested regular case (not WASM): works as expected, downloading the files work
I don't have setup for WASM

Copy link
Collaborator

@Ashod Ashod left a comment

Choose a reason for hiding this comment

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

Thanks @juliushaertl.

Looks good to me. @caolanm, any concerns?

@caolanm
Copy link
Contributor

caolanm commented Dec 7, 2023

no concerns, this seems to give us what we want without disrupting existing setups

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

No idea about wasm, but code changes look sensible.

which was added in: CollaboraOnline/online#7784

Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com>
@juliusknorr
Copy link
Member Author

Amended a small change to only set the allowEvalWasm if wasm support is indicated by the capabilities.

@caolanm
Copy link
Contributor

caolanm commented Dec 7, 2023

yes, sensible

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr merged commit 730c4cb into main Dec 7, 2023
58 checks passed
@juliusknorr juliusknorr deleted the enh/wasm-headers branch December 7, 2023 12:39
@Yetangitu
Copy link

These headers are added to all Nextcloud apps by richdocuments which causes the News app to fail to show feed icons. I solved this by commenting out that part of richdocuments but that's a crude hack. These headers should only be inserted when the richdocuments app is active and in use in the current session, not when other apps are in the foreground. As to how to achieve this I have not found a good solution yet but maybe I'm overlooking something?

@juliusknorr
Copy link
Member Author

Can you please file a new issue about this? It could be that we apply those too widely.

Just a sidenote: Wouldn't it be better to fetch the icons on the backend and cache them on the Nextcloud side? That way the request would be also hiding the individual user ips and avoid loading remote content directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants