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

hot-reloading doesn't remove old versions of content scripts #46

Conversation

cezaraugusto
Copy link

hey there! it seems webpack-target-webextension doesn't emove old versions of content scripts when using module.hot.accept(). This PR adds a sample that can reproduce. it seems an issue with both .js and .ts extensions.

Steps to Reproduce:

  • Pull the code
  • Edit the injected DOM in content.js
  • Actual Result:
    • one time: works;
    • two times: Injected DOM node piles up, console.logs too
  • Expected Result: Auto cleanup on file changes. Similar to how the react-hmr example works
Screenshot 2024-10-26 at 11 09 37

@@ -0,0 +1,37 @@
// Enable HMR if available
if (module.hot) {
Copy link
Author

Choose a reason for hiding this comment

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

to have it working I had to add a wrapper script that imports the module, but I'm not sure about the implications of using it with webpack-target-webextension or the react reloader for example so I appreciate any guidance

// minimum-content-script-file.js - Wrapper for user-defined content_scripts

function loadContentScript() {
  // Import content.js dynamically so HMR updates are tracked
  return import('./content.js')
}

// Content script initial load
let cleanup = loadContentScript()

// Enable HMR for the content script
if (module.hot) {
  module.hot.accept('./content.js', () => {
    // Remove previous content
    cleanup && cleanup()

    // Re-import the updated content script and run it again
    cleanup = loadContentScript()
  })

  // Ensure cleanup on dispose
  module.hot.dispose(() => {
    cleanup && cleanup()
  })
}

@Jack-Works
Copy link
Member

Hi! There is nothing wrong happened.

The wrapper is required because the outmost module does not support HMR. Things like React HMR won't happen because React HMR is based on many restrictions of the nature of React development. A React Component is stateless (all states are managed by the React runtime), and has no side effect (or having a cleanup function to cancel the side effect).

By using import.meta.webpackHot (or module.hot) directly, you're in full manual mode, you have to cleanup the side effects yourself. You can check https://webpack.js.org/guides/hot-module-replacement as a reference. This plugin only fixes HMR runtime in the Web Extension environment and does nothing else (it is also impossible to "undo" any code you have written).

@cezaraugusto
Copy link
Author

@Jack-Works thanks for taking a look. by the way, do you think my implementation is correct for the use-case of providing HMR for raw JS/TS files?

I want to provide Extension.js users a way to HMR raw JS/TS files, and also the reload of the core content_script file (when imported via manifest.json) for React/JS frameworks. for that, I'm considering a loader that under the hood wraps up the user code into a module that is imported within the code I shared above. I'd love to hear your thoughts on this

@Jack-Works
Copy link
Member

@Jack-Works thanks for taking a look. by the way, do you think my implementation is correct for the use-case of providing HMR for raw JS/TS files?

I want to provide Extension.js users a way to HMR raw JS/TS files, and also the reload of the core content_script file (when imported via manifest.json) for React/JS frameworks. for that, I'm considering a loader that under the hood wraps up the user code into a module that is imported within the code I shared above. I'd love to hear your thoughts on this

I can do some experiments with this, but I will work on other projects for now so I cannot answer this recently.

@cezaraugusto
Copy link
Author

@Jack-Works ok, sounds good!

@Jack-Works
Copy link
Member

hi! Have you tried experimental_output?

@cezaraugusto
Copy link
Author

hey @Jack-Works yes, I'd love your feedback on extension-js/extension.js#239.

I expected the following two test cases to be ok:

  1. Reloading the content_script index of the React example (file) reloads the page via HMR as usual
  2. Reloading the content_script index of any JS file declared reloads the page via live-reload or HMR

but after running experimental_outputs:

  1. Reloading the content_script index of the React example reloads the page infinitely and other files trigger the reload twice
  2. Same behavior as previous

happy to assist with testing/debugging. please let me know if these test cases should expect something else

@Jack-Works
Copy link
Member

hi! @cezaraugusto I have created an example PR of how to HMR the root of the content script. #56 Please let me know if you need more help and sorry for the months of delay!

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