Skip to content

fix: apply post hooks after serving .html files#14859

Closed
brillout wants to merge 2 commits intovitejs:mainfrom
brillout:brillout/fix-appType-mpa-posthooks
Closed

fix: apply post hooks after serving .html files#14859
brillout wants to merge 2 commits intovitejs:mainfrom
brillout:brillout/fix-appType-mpa-posthooks

Conversation

@brillout
Copy link
Copy Markdown
Contributor

@brillout brillout commented Nov 2, 2023

@bluwy Correct me if I'm wrong but this seems more correct.

@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Nov 2, 2023

But that isn't how dev works?

// html fallback
if (config.appType === 'spa' || config.appType === 'mpa') {
middlewares.use(htmlFallbackMiddleware(root, config.appType === 'spa'))
}
// run post config hooks
// This is applied before the html middleware so that user middleware can
// serve custom content instead of index.html.
postHooks.forEach((fn) => fn && fn())
if (config.appType === 'spa' || config.appType === 'mpa') {
// transform index.html
middlewares.use(indexHtmlMiddleware(root, server))

@brillout
Copy link
Copy Markdown
Contributor Author

brillout commented Nov 2, 2023

I've just checked and indexHtmlMiddleware() is a no-op if the .html file doesn't exist on the filesystem, so unless I'm missing something this seems to be a safe change. I've just udpated the PR to align dev.

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Nov 2, 2023

I don't understand why we need to change this though. It was working fine before. Post-middlewares could intercept HTML pages and do something with it, but now they can't and I don't see much reason to explain the change.

@brillout
Copy link
Copy Markdown
Contributor Author

brillout commented Nov 2, 2023

Makes sense 👍

@brillout brillout closed this Nov 2, 2023
@brillout brillout deleted the brillout/fix-appType-mpa-posthooks branch September 18, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants