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: apply post hooks after serving .html files #14859

Closed

Conversation

brillout
Copy link
Contributor

@brillout brillout commented Nov 2, 2023

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

Copy link

stackblitz bot commented Nov 2, 2023

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

@bluwy
Copy link
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
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
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
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