Jonghakseo / chrome-extension-boilerplate-react-vite

Chrome Extension Boilerplate with React + Vite + Typescript
MIT License
2.23k stars 324 forks source link

error on code reload #184

Closed PatrykKuniczak closed 10 months ago

PatrykKuniczak commented 1 year ago

Describe the bug Sometimes when i add some more code, e.g 50 lines, then i click inside plugin in chrome, and then i have error(scren shot section):

And i have this maybe once per 10/15 reloads.

Good to know is, i don't close plugin, popup is all time open, and after edit code i click inside plugin.

Almost all times works well, but sometimes don't want to reload.

Maybe it's related to #157

PS: When project grow up, then i one line change, may lead to an error.

To Reproduce Steps to reproduce the behavior:

  1. Clone and follow all steps for this repo reproduct.
  2. Create e.g 1 custm component
  3. Try to edit it, or paste several times in Popup.tsx

Screenshots Zrzut ekranu 2023-09-18 225639

For english: Can't access file. File could be moved. Editted or removed.

PS: This info is misleading, because user see, he have some error in plugin, but everything works fine.

I think this console.warn which display that, should be e.g console.info or sth like that, what don't be displayed as error in extenstions section.

image

image

Desktop (please complete the following information):

Jonghakseo commented 1 year ago

I'll take a look. :)

PatrykKuniczak commented 11 months ago

@Jonghakseo Any new info? :)

Jonghakseo commented 11 months ago

@PatrykKuniczak

Yeah, I can't reproduce it very well. :(

157 has been resolved, so I think it might have been resolved together, please check.

PatrykKuniczak commented 11 months ago

@PatrykKuniczak

Yeah, I can't reproduce it very well. :(

157 has been resolved, so I think it might have been resolved together, please check.

Ok, after work i will take a look :)

PatrykKuniczak commented 11 months ago

@Jonghakseo It's still occur

image

As you can see, it's default view

And i'm copying several times this switch theme button. I have all time open plugin in chrome.

And then after hmr build next time module, and it's finished.

Then i have this in chrome: image

After click outside(close plugin) and click again, to open, i have proper view: image

Jonghakseo commented 11 months ago

@PatrykKuniczak

Ok... It's weired... Can u reproduce this case on video?

https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/assets/53500778/66b116ea-59b6-4c80-bd94-138317fbe706

https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/assets/53500778/43d79dba-1147-4c6c-bac7-6e905518a306

https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/assets/53500778/deed45e9-4f18-42a5-99d9-99fa243b2920

PatrykKuniczak commented 11 months ago

As you can see i have it, still (I was pulled all new commits from main)

And for be sure, I turned it off all other plugins, but the bug still occur.

I see you're using Mac, i have this bug on Windows.

https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/assets/64608510/b7f1c87f-9a58-4055-a989-3879eb62327e

This errors is new, i don't see it before:

1 2 3 4

bambooom commented 11 months ago

Hi, it there any progress on this issue? I also has the same error for twind Screenshot 2023-10-22 at 16 31 14

PatrykKuniczak commented 11 months ago

For me it's now ocurr almost everytime

PatrykKuniczak commented 11 months ago

@Jonghakseo any moves forward?

vms-code commented 11 months ago

@bambooom @PatrykKuniczak the error you are referring to happens because the content page uses a shadow DOM and twind functions. The shadow DOM separates the environment from the main DOM, this breaks the CSS, helpful link: ShadowDom doc

So the way they make the CSS work is by adding the following function on the src/pages/content/components/Demo/index.tsx page: attachTwindStyle(rootIntoShadow, shadowRoot);

This function creates a new CSSStyleSheet(), then uses the twind function to add tailwind class rules to the stylesheet and finally uses the: documentOrShadowRoot.adoptedStyleSheets = [sheet.target]; line which injects the CSS rules into the shadow DOM.

if you want to use custom css rules instead of twind rules you can change the code to this:

export function attachTwindStyle<T extends { adoptedStyleSheets: unknown }>(
  observedElement: Element,
  documentOrShadowRoot: T,
) {
  //const sheet = cssom(new CSSStyleSheet());
  //const tw = twind(config, sheet);
  //observe(tw, observedElement);

  const customSheet = new CSSStyleSheet();

  // this are the same rules you would put in a normal .css file
  const cssRules = " body { background: #000; } .myClass { color: #fff; } ..." 

  customSheet.replaceSync(cssRules);
  documentOrShadowRoot.adoptedStyleSheets = [customSheet];
}

other useful link: replaceSync

PatrykKuniczak commented 11 months ago

background: #000; } .myClass { color: #fff; } ...\" \n \n customSheet.replaceSync(cssRules

I will take a look on that, i remove twid from my project, cause i was using mui with styled components, solution for this repo is probably remove twid completely, and style with pure tailwind, if the twid don't want to work in content page

Jonghakseo commented 10 months ago

@Jonghakseo any moves forward?

Nope.

Jonghakseo commented 10 months ago

background: #000; } .myClass { color: #fff; } ..." \n \n customSheet.replaceSync(cssRules

I will take a look on that, i remove twid from my project, cause i was using mui with styled components, solution for this repo is probably remove twid completely, and style with pure tailwind, if the twid don't want to work in content page

I agree. I'm going to remove the twind this weekend, and I'm thinking of configuring the solution that applies tailwind to be opt-in as a separate application method. I don't see any reason to put unnecessary code in for users who aren't going to use tailwind anyway.

Jonghakseo commented 10 months ago

I don't think the removal of TWIND will cause the warning, but the reload issue that was initially raised as an issue is hard to reproduce.

Due to my situation, it is difficult to access the window PC development environment... I hope someone can help :(

PatrykKuniczak commented 10 months ago

@Jonghakseo If i do

   socket.onerror = ev => {
    console.warn(ev);
  };

in initReloadClient.ts

then i can see an error, but it's sth strange:

image

Also still this error ocurr (more like info): image

This info is cause the socket is disconnected for a second, and lose refrerence or STH like that, then can't woke up after all modules are reloaded, i try to solve this, but i don't create this hot module, and i must see how it's built.

PatrykKuniczak commented 10 months ago

@Jonghakseo Probably we have 2 separate issue, but maybe it's cause after i working on #233 i don't remove node_modules and there's some conflict, i remove entirely node_modules and dist and i will try to handle this again, it's really strange bug xD

PatrykKuniczak commented 10 months ago

@Jonghakseo It's sth with that i think:

image

But its really hard to debug

PatrykKuniczak commented 10 months ago

image

I don't see it before, it's probably after add pnpm to the project, but i'm not sure.

But before we can use this template with trouble for time to time, but not it's completely useless, break on each/one to 2/3 code change, and not like before, only popup stop, and i must open it again, now the entire server is down, and i must reload it, that's big issue.

PatrykKuniczak commented 10 months ago

@Jonghakseo on your PC it works properly?

PatrykKuniczak commented 10 months ago

@Jonghakseo It's sth with view.ts

when on

function reload(): void {
    pendingReload = false;
    window.location.reload();
  }

reload window, then

 function reloadWhenTabIsVisible(): void {
    !document.hidden && pendingReload && reload();
  }

  document.addEventListener('visibilitychange', reloadWhenTabIsVisible);

Handle visibility change, and run it again, and sth like loop is played here, maybe i'm wrong, but for me it's a possibly that, i must try to reproduce this, i will update here if i'll find sth new.

PS: Now i see the if (document.hidden) on onUpdate, check if window is visible, and don't refresh when it's hidden ...

Jonghakseo commented 10 months ago

I found a similar issue in your error logs, but I don't know the cause yet. https://github.com/nodejs/node/issues/31702

Never mind, this doesn't seem to be relevant.

Jonghakseo commented 10 months ago

@PatrykKuniczak

Since it's a permissions issue anyway, how about assigning higher permissions to that folder?

As far as I can tell from the error log, it seems to be a permissions issue, which is necessary to remove existing files during the build process.

Jonghakseo commented 10 months ago

@Jonghakseo on your PC it works properly?

I don't have window PC... 😞

PatrykKuniczak commented 10 months ago

@Jonghakseo Maybe you can try on virtual env, sandbox or sth like that

PatrykKuniczak commented 10 months ago

@PatrykKuniczak

Since it's a permissions issue anyway, how about assigning higher permissions to that folder?

As far as I can tell from the error log, it seems to be a permissions issue, which is necessary to remove existing files during the build process.

You have any idea how to do it?

Jonghakseo commented 10 months ago

@Jonghakseo Maybe you can try on virtual env, sandbox or sth like that

You're right! I will try it.

PatrykKuniczak commented 10 months ago

@Jonghakseo I see apple have sth like that: https://support.apple.com/pl-pl/guide/mac-help/mh11850/mac

Install windows is better option, if u can, cause sometimes on virtual env sth can work in differen way, but after all virtual env is better than nothing :)

PatrykKuniczak commented 10 months ago

@Jonghakseo I solve this part with permission,

chokidar.watch('src', { ignorePermissionErrors: true })

and also for 'dist', this error with crashing views scripts also ocurr..

If u have any idea feel free to push #256

PatrykKuniczak commented 10 months ago

@Jonghakseo Theres issue with this error: https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/issues/100

this race condition is the key of this problem.

I see the solution, cause delay isn't good solution, cause we can't be confident, how much it can take to reload entire code.

We must reformat that, and when 1 'watch' process works, don't run next for avoid race condition, when dev make change during rebuilding, this changes will be taken after the previouse process is done or kill the process when dev make change during rebuilding, sth like that nestjs have implemented.

I think in that case, we should listent on both sides od websockets, when client side, send complete message, then on server side, unlock next rebuild, this could work, but performance could be weak, for better performance we could, kill this process and run next one.

Then we can remove entirely delay.

Maybe if this don't work, we can use chokidar events to handle that, e.g. on 'unlink' block next process and on 'ready' check for new changes.

If you have any idea, let share with me :)

I will take a look for that tomorrow.

PS: If any of this solution will work, then we can remove closing ws between rebuildings, cause this is also problematic.

I think we should establish 1 connection on start script and this WSS should run till, we or error shut down script(read node server ;))

Jonghakseo commented 10 months ago

@Jonghakseo Theres issue with this error: #100

this race condition is the key of this problem.

I see the solution, cause delay isn't good solution, cause we can't be confident, how much it can take to reload entire code.

We must reformat that, and when 1 'watch' process works, don't run next for avoid race condition, when dev make change during rebuilding, this changes will be taken after the previouse process is done or kill the process when dev make change during rebuilding, sth like that nestjs have implemented.

I think in that case, we should listent on both sides od websockets, when client side, send complete message, then on server side, unlock next rebuild, this could work, but performance could be weak, for better performance we could, kill this process and run next one.

Then we can remove entirely delay.

Maybe if this don't work, we can use chokidar events to handle that, e.g. on 'unlink' block next process and on 'ready' check for new changes.

If you have any idea, let share with me :)

I will take a look for that tomorrow.

PS: If any of this solution will work, then we can remove closing ws between rebuildings, cause this is also problematic.

I think we should establish 1 connection on start script and this WSS should run till, we or error shut down script(read node server ;))

@PatrykKuniczak

Cool! I think the biggest problem is that the dist folder is detected by the file system. I had an idea to send the build completion message from a custom plugin that is closely related to the build lifecycle, I'll try it. You're really great.