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(deno-deploy): decode static asset path before reading from filesystem #1494

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

nksaraf
Copy link
Contributor

@nksaraf nksaraf commented Jul 27, 2023

πŸ”— Linked issue

#1493

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When the browser requests a static asset: eg. /fonts/cera/Cera Pro.otf, it goes to readAsset in Deno where this happens

function readAsset (id) {
  // https://deno.com/deploy/docs/serve-static-assets
  const path = '.' + new URL(`../public${id}`, 'file://').pathname;
  return Deno.readFile(path);
}

This tries to read ./fonts/cera/Cera%20Pro.otf from the file system which ofcourse doesn't exist. We need to decodeURIComponent the pathname before using it to read the file.

Resolves #1493

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Review PR in StackBlitz Codeflow Submitted with StackBlitz Codeflow.

@nksaraf nksaraf changed the title fix(deno-assets): add decodeURIComponent before Deno tries to read an asset from filesystem fix(deno-deploy): add decodeURIComponent before Deno tries to read an asset from filesystem Jul 27, 2023
@nksaraf nksaraf changed the title fix(deno-deploy): add decodeURIComponent before Deno tries to read an asset from filesystem fix(deno-deploy): decodeURIComponent before Deno tries to read an asset from filesystem Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #1494 (929cd40) into main (888180b) will decrease coverage by 0.22%.
The diff coverage is 0.00%.

❗ Current head 929cd40 differs from pull request most recent head 8cbd75c. Consider uploading reports for the commit 8cbd75c to get more accurate results

@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
- Coverage   76.46%   76.25%   -0.22%     
==========================================
  Files          73       73              
  Lines        7577     7453     -124     
  Branches      752      733      -19     
==========================================
- Hits         5794     5683     -111     
+ Misses       1782     1769      -13     
  Partials        1        1              
Files Changed Coverage Ξ”
src/rollup/plugins/public-assets.ts 92.68% <0.00%> (ΓΈ)

... and 12 files with indirect coverage changes

@nuxt-studio
Copy link
Contributor

nuxt-studio bot commented Aug 7, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
nitro Edit on Studio β†—οΈŽ View Live Preview 8cbd75c

@pi0 pi0 changed the title fix(deno-deploy): decodeURIComponent before Deno tries to read an asset from filesystem fix(deno-deploy): decode static asset path before reading from filesystem Aug 7, 2023
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry took long to merge kinda missed it

@pi0 pi0 merged commit 4f4c017 into unjs:main Aug 7, 2023
5 checks passed
@pi0 pi0 mentioned this pull request Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deno deploy preset doesn't decodeURIComponent the file URL before reading from Deno.readFile
3 participants