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 download button #1937

Closed
wants to merge 1 commit into from

Conversation

ShadowRZ
Copy link
Contributor

@ShadowRZ ShadowRZ commented Sep 9, 2024

Description

Continuation of #1930, this time fix the download button.

Cinny uses file-saver to implment the download button, the file-saver implmentation has the following code:

https://github.com/eligrey/FileSaver.js/blob/5bb701bd6ea05a02836daf8ef88ec350a1dd4d83/src/FileSaver.js#L96-L98

which checks CORS using the following code:

https://github.com/eligrey/FileSaver.js/blob/5bb701bd6ea05a02836daf8ef88ec350a1dd4d83/src/FileSaver.js#L47-L55

In the earlier implmentation, HEAD request wasn't hooked, so when file-saver saves a file, it'll believe CORS request failed and try to open the URL in the browser tab, which won't work at all with authenicated media endpoints.

By hooking HEAD requests too, file-saver would properly save this file without opening a new tab.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Copy link

github-actions bot commented Sep 9, 2024

Preview: https://1937--pr-cinny.netlify.app
⚠️ Exercise caution. Use test accounts. ⚠️

@ajbura
Copy link
Member

ajbura commented Sep 10, 2024

I reviewed it, and it looks like file-saver still open them in new tab with /media/download url which is out of the scope of our service worker. so maybe a good fix will be if we change file-saver usage with custom download login?

@ShadowRZ
Copy link
Contributor Author

Probably, yes. Maybe telling file-saver to download a blob is better.

@kfiven
Copy link
Collaborator

kfiven commented Sep 11, 2024

Closing in favour of #1947

@kfiven kfiven closed this Sep 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants