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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 BUG: HMR destroy scripts (and changes made) because they don't re-run on HMR update #3556

Closed
1 task
felixsanz opened this issue Jun 8, 2022 · 3 comments 路 Fixed by #3932
Closed
1 task
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) feat: hmr Related to HMR (scope)

Comments

@felixsanz
Copy link
Contributor

felixsanz commented Jun 8, 2022

What version of astro are you using?

v1.0.0-beta.42

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Linux

Describe the Bug

Inserting scripts that make changes to the page (DOM), like this...

    <script is:inline>
      const link = document.createElement('link')
      link.rel = 'stylesheet'
      link.href = `/themes/default.css`

      document.head.insertAdjacentElement('afterbegin', link)
    </script>

...are lost on HMR because HMR reloads the entire page and scripts are not re-run because they hasn't changed.

As discussed on discord, a solution could be a new directive like <script is:fixed> that re-runs the script on HMR and is stripped off on building. HMR algorithm should just re-run those scripts like if they were changed

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-rqmsac-zbs3uf

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added this to Needs Triage in 馃悰 Bug Tracker Jun 8, 2022
@natemoo-re
Copy link
Member

Chatted about this on Discord. We probably need some way for authors to specify that they want certain scripts to rerun when HMR happens, unless we can find a way scope HMR updates to component boundaries.

Will need to discuss this with the team.

@natemoo-re natemoo-re moved this from Needs Triage to Accepted in 馃悰 Bug Tracker Jun 8, 2022
@natemoo-re natemoo-re added s2-medium needs discussion Issue needs to be discussed and removed s2-medium labels Jun 8, 2022
@tony-sull
Copy link
Contributor

Community Call 6/21

  • Could be really interesting to have an option to opt-out of HMR for live reload. This may also be one specific case we need to just recognize on the page and trigger a full reload when <script is:inline> is used

  • Further discussion - It is worth keeping an eye on the value add of HMR vs. the cost of maintenance when it is often impacted by the use of specific features and component frameworks

  • Could Vite's HMR APIs be a nice solution here?

@natemoo-re natemoo-re added - P4: important Violate documented behavior or significantly impacts performance (priority) feat: hmr Related to HMR (scope) and removed needs discussion Issue needs to be discussed labels Jun 29, 2022
@natemoo-re natemoo-re self-assigned this Jul 7, 2022
@natemoo-re
Copy link
Member

Marking this as closed by #3932. We'll now perform a full reload if scripts are present.

@natemoo-re natemoo-re linked a pull request Jul 19, 2022 that will close this issue
馃悰 Bug Tracker automation moved this from Accepted to Done Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) feat: hmr Related to HMR (scope)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants