feat: handle static assets in case-sensitive manner by benmccann · Pull Request #10475 · vitejs/vite

benmccann · 2022-10-14T20:05:33Z

Description

Enforce case sensitivity so that your code works if you develop on Mac / Windows and then deploy to Linux. It also becomes easier to work on a code base where people on the team are using different OS.

Closes #9260

Additional context

Discussed in #contributing on Oct 14: https://discord.com/channels/804011606160703521/804439875226173480/1030501340373336144

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the Commit Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

benmccann · 2022-10-15T03:04:50Z

Hmm. I might need some help with this one. The test passes on Linux, which is what I run, so this will be very difficult for me to get that test working since I don't have a way to reproduce the error

dominikg · 2022-10-15T12:03:34Z

the check "file exists with correct case" on case insensitive fs may be usefule outside of the static middleware so maybe move it to a util and add a unit test too?

there is

vite/packages/vite/src/node/utils.ts

Line 218 in ee3231c

export const isCaseInsensitiveFS = testCaseInsensitiveFS()

that also uses existsSync to test for case insensitivity.

The current implementation recurses to check all parent directory names separately, doing readDir calls to list files. This could be a bit more expensive on the disk operation side, so may be worth caching the positive results in a set?

dominikg · 2022-10-15T13:35:49Z

The error response is

{
  name: 'RAW.js',
  status: 500,
  text: '\n' +
    '        <!DOCTYPE html>\n' +
    '        <html lang="en">\n' +
    '          <head>\n' +
    '            <meta charset="UTF-8" />\n' +
    '            <title>Error</title>\n' +
    '            <script type="module">\n' +
    "              import { ErrorOverlay } from '/@vite/client'\n" +
    '              document.body.appendChild(new ErrorOverlay({"message":"Failed to load url /RAW.js (resolved id: /RAW.js). This file is in /public and will be copied as-is during build without going through the plugin transforms, and therefore should not be imported from source code. It can only be referenced via HTML tags.","stack":"    at loadAndTransform (file:///C:/Users/IEUser/dev/vite/packages/vite/dist/node/chunks/dep-f1192f4f.js:37817:19)\\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)","frame":""}))\n' +
    '            </script>\n' +
    '          </head>\n' +
    '          <body>\n' +
    '          </body>\n' +
    '        </html>\n' +
    '      '
}

which seems like some extra handling for script files in public dir.

If you use icon.png and ICON.png instead the test passes on windows at least

benmccann · 2022-10-15T18:02:08Z

Thank you so much for taking a look at this!

If you use icon.png and ICON.png instead the test passes on windows at least

Are you sure? I just changed it to that and the CI is failing still. Though now it gets a 200 rather than 500, which I suppose is an improvement.

It's curious because I stole this code from SvelteKit and it has a test for it that passes.

dominikg · 2022-10-15T19:42:04Z

The previous fail was during test-serve, the current one is on test-build. Unfortunately i didn't run the full suite after finding the first problem. Sorry, 🙈
I guess the build tests use vite preview or another static server to serve the build output and that doesn't use the middleware you changed in this PR.

playground/assets/__tests__/assets.spec.ts

benmccann · 2022-10-17T05:08:37Z

Ah, thanks!! I really appreciate the help @dominikg! I've implemented it for preview as well so the tests are passing now!

benmccann · 2022-10-17T16:58:25Z

I moved the method to utils.ts as part of making it shared between dev and preview. I'm not sure it needs to be made more general until there's a usecase for it though.

I also leveraged the isCaseInsensitiveFS variable you mentioned to skip the check on Linux. I'm not too worried about optimizing performance beyond that though because it only affects dev/preview and not prod and this functionality has been in SvelteKit for a long time and has never caused any perf issues there.

BenediktAllendorf · 2022-10-19T16:36:24Z

I had a look at the Todo about those double slashes:

First of all, the behaviour (double slashes) is not new. Apparently sirv() didn't care but shouldServe() does.

They occur because viteTestUrl is set here to something like 'http://localhost:4173/' (with trailing slash) and many (most? all?) URLs in the tests are build like await page.goto(viteTestUrl + '/something'), leading to duplicated slashes.

So either change all the tests not to include that slash, change viteTestUrl to not include the trailing slash, or make shouldServe() handle double slashes as well.

benmccann · 2022-11-02T17:36:37Z

So either change all the tests not to include that slash, change viteTestUrl to not include the trailing slash, or make shouldServe() handle double slashes as well.

Thanks! I added it to shouldServe

bluwy · 2022-11-04T14:23:42Z

We discussed this in the last team meeting and agree with this. Just needs a review and it's good to go.

packages/vite/src/node/utils.ts

bluwy · 2022-11-11T05:49:16Z

packages/vite/src/node/utils.ts

+  const pathname = decodeURIComponent(
+    new URL(url.startsWith('//') ? url.substring(1) : url, 'http://example.com')
+      .pathname
+  )


Looks like sirv is able to handle // because it uses @polka/url. We could probably use that too as it caches the result to req, but also something we can improve later.

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

benmccann · 2022-11-11T18:40:59Z

Thanks for the review @bluwy! I've addressed your comments

patak-cat

Looks good to me. Let's merge so we give more time to other projects to test this.

ElMassimo · 2022-12-09T15:15:25Z

Testing Vite 4 with îles.

An unintended effect of #10475 is that sirv will only receive requests to /existing if there's an existing/index.html file, while prior to the change sirv was able to resolve /existing to existing.html file.

This is important to be able to locally preview sites that will be deployed using cleanUrls in Vercel, or in Netlify.

Ideally, frameworks like îles should be able to override shouldServe, as it limits the usefulness of the preview command.

benmccann force-pushed the case-sensitive branch from c6c2da6 to e830803 Compare October 15, 2022 17:50

dominikg reviewed Oct 15, 2022

View reviewed changes

playground/assets/__tests__/assets.spec.ts Show resolved Hide resolved

sapphi-red added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Oct 17, 2022

benmccann force-pushed the case-sensitive branch from bed03db to e6846e3 Compare October 17, 2022 15:40

benmccann mentioned this pull request Oct 17, 2022

Dev server does not apply base path correctly sveltejs/kit#2958

Closed

benmccann force-pushed the case-sensitive branch from e6846e3 to 77b51ad Compare October 17, 2022 16:26

feat: handle static assets in case-sensitive manner

118e36d

benmccann force-pushed the case-sensitive branch from 77b51ad to 118e36d Compare October 17, 2022 16:43

benmccann added the breaking change label Oct 18, 2022

patak-cat added this to the 4.0 milestone Oct 21, 2022

benmccann added 2 commits October 21, 2022 10:29

chore: cleanup todo

f5d053f

chore: merge main

b4b2dc6

bluwy reviewed Nov 11, 2022

View reviewed changes

benmccann and others added 2 commits November 11, 2022 08:23

Update packages/vite/src/node/utils.ts

1e6e834

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

chore: use path.join

14a0b50

patak-cat approved these changes Nov 12, 2022

View reviewed changes

patak-cat merged commit c1368c3 into vitejs:main Nov 12, 2022

benmccann deleted the case-sensitive branch November 12, 2022 19:49

benmccann mentioned this pull request Nov 13, 2022

fix: don't throw on malformed URLs #10901

Merged

9 tasks

fc mentioned this pull request Nov 14, 2022

Routing broken for preview with react-router-dom in 4.0.0-alpha.2 #10925

Closed

7 tasks

brc-dd mentioned this pull request Sep 17, 2023

url become case sensitive after build vuejs/vitepress#2343

Closed

4 tasks

Uh oh!

Conversation

benmccann commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

benmccann commented Oct 15, 2022

Uh oh!

dominikg commented Oct 15, 2022

Uh oh!

dominikg commented Oct 15, 2022

Uh oh!

benmccann commented Oct 15, 2022

Uh oh!

dominikg commented Oct 15, 2022

Uh oh!

Uh oh!

benmccann commented Oct 17, 2022

Uh oh!

benmccann commented Oct 17, 2022

Uh oh!

BenediktAllendorf commented Oct 19, 2022

Uh oh!

benmccann commented Nov 2, 2022

Uh oh!

bluwy commented Nov 4, 2022

Uh oh!

Uh oh!

Uh oh!

bluwy Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

benmccann commented Nov 11, 2022

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

ElMassimo commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

benmccann commented Oct 14, 2022 •

edited

Loading

ElMassimo commented Dec 9, 2022 •

edited

Loading