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: prevent unnecessary reloads #8356

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sibbng
Copy link
Contributor

@sibbng sibbng commented May 27, 2022

Description

I'm working on a Tailwind project, when I update an html file that is tracked by Tailwind, Vite full-reloads all pages.

If files are tracked by Tailwind they bypass this check:

if (!hmrContext.modules.length) {

In my case, hmrContext.modules is:

hmrContext.modules
[
  ModuleNode {
    id: 'C:/Users/sib/dev/luidev/components/HTML/default.html',  
    file: 'C:/Users/sib/dev/luidev/components/HTML/default.html',
    importers: Set(1) {
      ModuleNode {
        id: 'C:/Users/sib/dev/luidev/server/style.css',
        file: 'C:/Users/sib/dev/luidev/server/style.css',        
        importers: [Set],
        importedModules: [Set],
        acceptedHmrDeps: Set(0) {},
        transformResult: [Object],
        ssrTransformResult: null,
        ssrModule: null,
        ssrError: null,
        lastHMRTimestamp: 1653670227215,
        lastInvalidationTimestamp: 0,
        url: '/style.css',
        type: 'js',
        isSelfAccepting: true
      }
    },
    importedModules: Set(0) {},
    acceptedHmrDeps: Set(0) {},
    transformResult: null,
    ssrTransformResult: null,
    ssrModule: null,
    ssrError: null,
    lastHMRTimestamp: 0,
    lastInvalidationTimestamp: 1653670238242,
    url: '/@fs/C:/Users/sib/dev/luidev/components/HTML/default.html',
    type: 'js',
    isSelfAccepting: false
  }
]

When they bypass here, they fall into here:

if (needFullReload) {
config.logger.info(colors.green(`page reload `) + colors.dim(file), {
clear: true,
timestamp: true
})
ws.send({
type: 'full-reload'
})

Which is the source of problem. This part of code doesn't send path to full-reload events and this cause full reload on all pages:

if (payload.path && payload.path.endsWith('.html')) {
// if html file is edited, only reload the page if the browser is
// currently on that page.
const pagePath = decodeURI(location.pathname)
const payloadPath = base + payload.path.slice(1)
if (
pagePath === payloadPath ||
payload.path === '/index.html' ||
(pagePath.endsWith('/') && pagePath + 'index.html' === payloadPath)
) {
location.reload()
}
return
} else {
location.reload()
}

I assume this is what happened to @cdauth, as he described in #6695:

form state to be lost whenever I edit some unrelated file

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

@sapphi-red sapphi-red added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jun 16, 2022
@sapphi-red
Copy link
Member

I guess changing

  if (!hmrContext.modules.length) {
    // html file cannot be hot updated
    if (file.endsWith('.html')) {
      config.logger.info(colors.green(`page reload `) + colors.dim(shortFile), {
        clear: true,
        timestamp: true
      })
      ws.send({
        type: 'full-reload',
        path: config.server.middlewareMode
          ? '*'
          : '/' + normalizePath(path.relative(config.root, file))
      })
    } else {
      // loaded but not in the module graph, probably not js
      debugHmr(`[no modules matched] ${colors.dim(shortFile)}`)
    }
    return
  }

to

  // html file cannot be hot updated
  if (file.endsWith('.html')) {
    config.logger.info(colors.green(`page reload `) + colors.dim(shortFile), {
      clear: true,
      timestamp: true
    })
    ws.send({
      type: 'full-reload',
      path: config.server.middlewareMode
        ? '*'
        : '/' + normalizePath(path.relative(config.root, file))
    })
    return
  }

  if (!hmrContext.modules.length) {
    // loaded but not in the module graph, probably not js
    debugHmr(`[no modules matched] ${colors.dim(shortFile)}`)
    return
  }

is better. (I'm not sure if I'm correct.)

@typedarray
Copy link

I'm running into a slightly different issue, but the fix is the same. Is there a good reason not to add the path that caused the full-reload to the 'full-reload' event payload? In my case, I'm experimenting with vite-node and would like to know the path that caused the full reload to construct a nice log message.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I think the current PR is fine by me, but probably be safer if we do it like this too:

hot.send({
type: 'full-reload',
path: config.server.middlewareMode
? '*'
: '/' + normalizePath(path.relative(config.root, file)),
})

Regarding sapphi's comment, I suppose if the HTML was ever in the hmrContext.modules, it may be safer to invalidate the modules anyway to prevent potential issues with tailwind.

@sapphi-red
Copy link
Member

Regarding sapphi's comment, I suppose if the HTML was ever in the hmrContext.modules, it may be safer to invalidate the modules anyway to prevent potential issues with tailwind.

With the updated understanding over the last two years, I agree with that. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants