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

HMR does not work properly with class components #75

Closed
UROjQ6r80p opened this issue Jul 18, 2023 · 14 comments · Fixed by #124
Closed

HMR does not work properly with class components #75

UROjQ6r80p opened this issue Jul 18, 2023 · 14 comments · Fixed by #124

Comments

@UROjQ6r80p
Copy link

Version: x.x.x

2.4.8

Details

HMR does not work properly with class components.

I use npx @marko/create -> vite-express -> npm install @marko/vite@latest

HMR does not work properly with class components.
In vite-express starter, by default there is component app-button if i try to add random <span>test</span> to it, the console shows:
[vite] hmr update /src/components/app-button/index.marko?marko-browser, /src/components/app-button/index.marko
but component isn't reloaded correctly in the browser. It shows the outdated version even after manually refreshing the browser.

For components without class, for example app-layout it seems to always work, and then console shows [vite] page reload src/components/app-layout/index.marko

Expected Behavior

Component should be replaced to the new version

Actual Behavior

above.

Possible Fix

Additional Info

Your Environment

  • Environment name and version (e.g. Chrome 39, node.js 5.4):
  • Operating System and version (desktop or mobile):
  • Link to your project:

Steps to Reproduce

  1. first...

Stack Trace

@DylanPiercey
Copy link
Contributor

@UROjQ6r80p I can't seem to reproduce this myself. Could you share the result of running:

npm ls marko @marko/compiler @marko/vite vite

Also what operating system and browser you are using?

@UROjQ6r80p
Copy link
Author

UROjQ6r80p commented Jul 18, 2023

@DylanPiercey

Before I've tried the latest version of marko packages and vite.
But it also does not work properly right after using npx @marko/create -> vite-express

Here is the result:

├── @marko/[email protected]
├─┬ @marko/[email protected]
│ └── [email protected] deduped
├─┬ @marko/[email protected]
│ ├── @marko/[email protected] deduped
│ └── [email protected] deduped
├─┬ [email protected]
│ ├── @marko/[email protected] deduped
│ └─┬ @marko/[email protected]
│   ├── @marko/[email protected] deduped
│   └── [email protected] deduped
└── [email protected]

System: Windows 10. Browser: Brave, but also tried Chrome, Firefox, Opera.

Basically what I'm doing is constantly adding few letters in the button, then removing it, then adding, then removing, and it always shows outdated component.

hmr.mp4

I've tried to reproduce it on StackBlitz https://stackblitz.com/edit/github-fxezeu?file=src%2Fcomponents%2Fapp-button%2Findex.marko and it seems to work without problems so i guess it's related to Windows specifically.

Additionally, for example this url:
http://localhost:3000/src/components/app-button/index.marko
after I change the component and save file, this url always show up to date component.
but this:
http://localhost:3000/src/components/app-button/index.marko?marko-browser shows outdated version.

I tried to locally change this line at https://github.com/marko-js/vite/blob/main/src/index.ts

async load(id) {
        const query = getMarkoQuery(id);
        switch (query) {
          case serverEntryQuery: {
            entryIds.add(id.slice(0, -query.length));
            return null;
          }
          case browserEntryQuery:
          case browserQuery: {
            // COMMENTED THIS LINE return cachedSources.get(id.slice(0, -query.length)) || null;
            return virtualFiles.get(id) || null; // ADDED THIS
          }
        }

        return virtualFiles.get(id) || null;
      }

and after this change it seems to always show updated version, but i dont know how it affects the rest of the code.

@danbarbarito
Copy link

I'm running into this issue as well. Also on Windows.

@danbarbarito
Copy link

Actually @UROjQ6r80p I was just able to fix this. I turned off "linked" mode and it seemed to work now. At the root of my project, I have a vite.config.js with the following:

import { defineConfig } from "vite";
import marko from "@marko/run/vite";

export default defineConfig({
  plugins: [
    marko({
      linked: false,
    }),
  ],
});

@DylanPiercey
Copy link
Contributor

@danbarbarito by turning linked: off you opt into exclusively server side rendering (or client rendering depending on how you configured vite).

There may well be a windows specific issue here, I need to take some time to check 😓.
The mentioned line by @UROjQ6r80p https://github.com/marko-js/vite/blob/main/src/index.ts#L559 should be invalidated on change through vites watcher here https://github.com/marko-js/vite/blob/main/src/index.ts#L400

I wonder if it's got differently normalized file names here 🤔

@DylanPiercey
Copy link
Contributor

@danbarbarito I think it may work if we change https://github.com/marko-js/vite/blob/main/src/index.ts#L400 to
cachedSources.delete(normalizePath(filename));. Are you able to make that change in your node_modules and see if it works for you?

@danbarbarito
Copy link

@DylanPiercey My node_modules only has the dist directory available in @marko/vite. If you know of a quick way to pull down the source and use that instead please let me know. I'll try and figure it out in the meantime

@DylanPiercey
Copy link
Contributor

@danbarbarito probably simplest to just edit the dist.
Could you open node_modules/@marko/vite/dist/index.mjs and search for cachedSources.delete(filename)? It should be line 897

@danbarbarito
Copy link

@DylanPiercey That didnt work for me unfortunately 😢

@DylanPiercey
Copy link
Contributor

Shoot. I'll have to get a windows vm setup again so I can test properly.

@danbarbarito
Copy link

Ok, I appreciate you looking into it. If its easier for us to get on a call and have me share my screen as you tell me the changes to make just let me know.

@danbarbarito
Copy link

Wait I messed up before! It works @DylanPiercey ! I guess I didn't restart Vite before or something. Sorry for the confusion.

@DylanPiercey
Copy link
Contributor

Oh that's awesome. Allow me to do a quick release then!

@DylanPiercey
Copy link
Contributor

Should be fixed in 4.1.7

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 a pull request may close this issue.

3 participants