feat!: allow path containing . to fallback to index.html by sapphi-red · Pull Request #14142 · vitejs/vite

sapphi-red · 2023-08-17T15:59:47Z

Description

This PR sets disableDotRule: true to connect-history-api-fallback.
I believe threre's no reason to set this to false for Vite.

fixes #2415
supersedes close #2634

Additional context

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 PR Title 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.

bolt-new-by-stackblitz · 2023-08-17T15:59:51Z

Run & review this pull request in StackBlitz Codeflow.

sapphi-red · 2023-08-17T16:43:04Z

packages/vite/src/node/server/middlewares/htmlFallback.ts

+      // don't rewrite paths ending with .html
+      {
+        from: /\.html$/,
+        to({ request }: any) {
+          return request.url
+        },
+      },


Because serveStaticMiddleware doesn't handle html files and the fallback middleware runs before html middleware.

vite/packages/vite/src/node/server/middlewares/static.ts

Lines 91 to 102 in bd9b749

// only serve the file if it's not an html request or ends with `/`

// so that html requests can fallthrough to our html middleware for

// special processing

// also skip internal requests `/@fs/ /@vite-client` etc...

const cleanedUrl = cleanUrl(req.url!)

if (

cleanedUrl[cleanedUrl.length - 1] === '/' ||

path.extname(cleanedUrl) === '.html' ||

isInternalRequest(req.url!)

) {

return next()

}

sapphi-red · 2023-08-17T16:43:58Z

playground/assets/__tests__/assets.spec.ts

+  expect((await fetchPath('ICON.png')).headers.get('Content-Type')).toBe(
+    isBuild ? 'text/html; charset=utf-8' : 'text/html',
+  )

-  expect((await fetchPath('bar')).status).toBe(200)
+  expect((await fetchPath('bar')).headers.get('Content-Type')).toBe('')
  // fallback to index.html
-  const incorrectBarFetch = await fetchPath('BAR')
-  expect(incorrectBarFetch.status).toBe(200)
-  expect(incorrectBarFetch.headers.get('Content-Type')).toContain('text/html')
+  expect((await fetchPath('BAR')).headers.get('Content-Type')).toContain(
+    isBuild ? 'text/html;charset=utf-8' : 'text/html',
+  )


I haven't checked why one is text/html; charset=utf-8 (with space) and the other is text/html;charset=utf-8 (without space). 🤔

sapphi-red · 2023-08-17T16:45:59Z

playground/legacy/__tests__/legacy.spec.ts

-          'index: 404',
-          'index-legacy: 404',
-          'chunk-async: 404',
-          'chunk-async-legacy: 404',
-          'immutable-chunk: 200',
-          'immutable-chunk-legacy: 200',
-          'polyfills-legacy: 404',
+          'index: text/html; charset=utf-8',
+          'index-legacy: text/html; charset=utf-8',
+          'chunk-async: text/html; charset=utf-8',
+          'chunk-async-legacy: text/html; charset=utf-8',
+          'immutable-chunk: application/javascript',
+          'immutable-chunk-legacy: application/javascript',
+          'polyfills-legacy: text/html; charset=utf-8',
        ].join('\n')
      : [
-          'index: 404',
-          'index-legacy: 404',
-          'chunk-async: 404',
-          'chunk-async-legacy: 404',
-          'immutable-chunk: 404',
-          'immutable-chunk-legacy: 404',
-          'polyfills-legacy: 404',
+          'index: text/html',
+          'index-legacy: text/html',
+          'chunk-async: text/html',
+          'chunk-async-legacy: text/html',
+          'immutable-chunk: text/html',
+          'immutable-chunk-legacy: text/html',
+          'polyfills-legacy: text/html',


sirv adds ; charset=utf-8 to html files (lukeed/sirv#122). But in vite dev, Vite doesn't.
Maybe we should add it?
I didn't reproduce Firefox/Chromium showing a warning described in lukeed/sirv#106 though.

I remember seeing some issues regarding charsets too, but I think it's better to leave no charset in dev. Not all prod servers at ; charset=utf-8 and some might not want that, so it would be safer for us to not decide on behalf.

playground/legacy/index.html

bluwy · 2023-08-18T16:43:49Z

playground/legacy/__tests__/legacy.spec.ts

-          'index: 404',
-          'index-legacy: 404',
-          'chunk-async: 404',
-          'chunk-async-legacy: 404',
-          'immutable-chunk: 200',
-          'immutable-chunk-legacy: 200',
-          'polyfills-legacy: 404',
+          'index: text/html; charset=utf-8',
+          'index-legacy: text/html; charset=utf-8',
+          'chunk-async: text/html; charset=utf-8',
+          'chunk-async-legacy: text/html; charset=utf-8',
+          'immutable-chunk: application/javascript',
+          'immutable-chunk-legacy: application/javascript',
+          'polyfills-legacy: text/html; charset=utf-8',


Should we leave them 404 in appType: 'mpa'? It would be odd if they fallback to the index.html 🤔

Legacy playground is not appType: 'mpa'. This part is the result of fetching all chunks.
https://github.com/vitejs/vite/pull/14142/files/79491351b244ee9ceff9cccbacb810fe47744b28#diff-b17ad79ba0d84b7069324d7eb460e718df66793c5cc903f5b6ff92b5adb73c20

Yeah the test here is fine since it's an spa, but for MPAs (which I manually tested) it also has this same result, and I wonder if that's correct. For example, if we fetch index (which doesn't exist), in SPA it should fallback to /index.html, but in MPA, it should 404 (no Content-Type) instead?

Ah, I see. That behavior was there before this PR.
This PR just makes the paths with . work like the paths without ..

Access Response before this PR Response after this PR

/ /index.html /index.html

/foo /index.html /index.html

/foo/ 404 404

/foo.png 404 /index.html

/nested /index.html /index.html

/nested/ /nested/index.html /nested/index.html

/nested/foo /index.html /index.html

/nested/foo.png 404 /index.html

(in MPA)

Thanks. My bad too, I thought the 404 before was an actual 404 without index.html. Thanks for the table that makes it clearer that this is fine for now 👍

patak-cat

We discussed this PR in today's team meeting, and decided to move forward with it

vitejs/vite#14142

btea · 2023-09-07T14:45:38Z

@sapphi-red @bluwy This PR has caused some questions so far.

For example, when I use the img tag to load an image on the page if I write the wrong path, the console will print a 404 error before this PR, but the console will not print any information after this PR. This is very unfriendly to development prompts.

In my opinion, this impact seems to be quite large, what do you think? 🤔

bluwy · 2023-09-08T05:07:22Z

@btea I think that should be expected since it's an SPA, and you should be able to opt-out of that with appType: 'mpa', but currently even in MPA it doesn't work (as discussed in #14142 (comment)).

It looks like this happens because of #9865. The proper fix would be something on the last line of this comment: #9865 (comment)

We also can't revert this as the linked issue #2415 had a lot of request, and between two tradeoffs, I think it's better to lean into this PR instead.

sapphi-red · 2023-09-08T07:59:57Z

Rewriting only when Accept header includes text/html might work. If many static hosting service does that, I think it makes sense to have that.
But I'm not sure how much apps will break if we do that.

btea · 2023-09-08T12:41:09Z

Thank you for your replies! Regarding the problems that may be caused by the PR modification I mentioned, perhaps a detailed explanation should be added to the official website.

Or is it possible to provide users with relevant configurations to actively set fallback policies?

feat!: allow path containing . to fallback to index.html

05fb60e

sapphi-red added p2-to-be-discussed Enhancement under consideration (priority) breaking change labels Aug 17, 2023

sapphi-red marked this pull request as draft August 17, 2023 16:00

sapphi-red added 2 commits August 18, 2023 01:37

fix: don't rewrite *.html

67232cc

test: use content-type instead of status code

dd7ae92

sapphi-red marked this pull request as ready for review August 17, 2023 16:41

sapphi-red commented Aug 17, 2023

View reviewed changes

chore: remove unnecessary diff

7949135

bluwy reviewed Aug 18, 2023

View reviewed changes

bluwy approved these changes Aug 21, 2023

View reviewed changes

patak-cat added this to the 5.0 milestone Aug 24, 2023

sapphi-red requested a review from patak-cat August 24, 2023 10:37

patak-cat approved these changes Aug 24, 2023

View reviewed changes

patak-cat merged commit 1ae4cbd into vitejs:main Aug 24, 2023

dschmidt mentioned this pull request Aug 25, 2023

Vite: Enable history mode in Dev Server owncloud/web#8098

Closed

2 tasks

sapphi-red deleted the feat/disable-dot-rule-by-default branch August 25, 2023 16:16

wallpants added a commit to wallpants/github-preview.nvim that referenced this pull request Sep 6, 2023

install vite 5

947e2a6

vitejs/vite#14142

wallpants added a commit to wallpants/github-preview.nvim that referenced this pull request Sep 6, 2023

install vite 5

226e788

vitejs/vite#14142

wallpants added a commit to wallpants/github-preview.nvim that referenced this pull request Sep 6, 2023

install vite 5

0f01164

vitejs/vite#14142

sapphi-red mentioned this pull request Sep 23, 2023

docs: v4 to v5 migration guide #14343

Merged

9 tasks

bluwy mentioned this pull request Sep 25, 2023

feat(server): file request response content type contains utf-8 #14453

Closed

9 tasks

silverwind mentioned this pull request Nov 20, 2023

Deprecate package? ivesia/vite-plugin-rewrite-all#6

Closed

sealer3 mentioned this pull request Nov 24, 2023

Bump vite from 4.5.0 to 5.0.2 otterscan/otterscan#1499

Merged

tiagofilipenunes mentioned this pull request Jul 3, 2024

[Test] Setup integration tests bancorprotocol/carbon-app#1323

Merged

	// only serve the file if it's not an html request or ends with `/`
	// so that html requests can fallthrough to our html middleware for
	// special processing
	// also skip internal requests `/@fs/ /@vite-client` etc...
	const cleanedUrl = cleanUrl(req.url!)
	if (
	cleanedUrl[cleanedUrl.length - 1] === '/' \|\|
	path.extname(cleanedUrl) === '.html' \|\|
	isInternalRequest(req.url!)
	) {
	return next()
	}

Access	Response before this PR	Response after this PR
`/`	`/index.html`	`/index.html`
`/foo`	`/index.html`	`/index.html`
`/foo/`	404	404
`/foo.png`	404	`/index.html`
`/nested`	`/index.html`	`/index.html`
`/nested/`	`/nested/index.html`	`/nested/index.html`
`/nested/foo`	`/index.html`	`/index.html`
`/nested/foo.png`	404	`/index.html`

Uh oh!

Conversation

sapphi-red commented Aug 17, 2023

Description

Additional context

What is the purpose of this pull request?

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

Uh oh!

bolt-new-by-stackblitz bot commented Aug 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluwy Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

btea commented Sep 7, 2023

Uh oh!

bluwy commented Sep 8, 2023

Uh oh!

sapphi-red commented Sep 8, 2023

Uh oh!

btea commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bluwy Aug 18, 2023 •

edited

Loading