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

ViewTransition - Identifier 'theme' has already been declared #8466

Closed
1 task done
visualnaut opened this issue Sep 8, 2023 · 11 comments · Fixed by #8603
Closed
1 task done

ViewTransition - Identifier 'theme' has already been declared #8466

visualnaut opened this issue Sep 8, 2023 · 11 comments · Fixed by #8603
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) feat: view transitions Related to the View Transitions feature (scope)

Comments

@visualnaut
Copy link

Astro Info

Astro                    v3.0.7
Node                     v18.17.1
System                   macOS (arm64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             @astrojs/mdx

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When using ViewTransition on global layout with a astro component that have inline script, it somehow logged a syntax error every time we traverse a page and the inline script did not worked.

I search in the issues list and I found old similar case but with different root cause #3643

What's the expected result?

Inline script inside astro component worked and no syntax error being logged.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-wqkwwm

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Sep 8, 2023
@dawichi
Copy link

dawichi commented Sep 8, 2023

Exactly the same error here with ViewTransition

I have a blog with just a homepage pages/index.astro and 2 posts (content) visibles under [...slug].astro. With tailwind and mdx, nothing fancy, just wanted to play with this viewtransitions thing

After set the <ViewTransitions/> in the head, that error appeared each time i change the page.

VM2713:1  Uncaught SyntaxError: Identifier 'theme' has already been declared
    at runScripts (ViewTransitions.astro:102:11)
    at navigate (ViewTransitions.astro:276:10)

Also, this is my component, nothing fancy neither

<button id="dark-toggle" class="bg-gray-300 dark:bg-zinc-700 rounded-full px-2 py-1">
    <i class="bi bi-moon-fill" />
</button>

<script is:inline>
    function toggleDarkMode() {
        const newMode = localStorage.getItem('theme') === 'dark' ? 'light' : 'dark'
        document.body.dataset.theme = newMode
        localStorage.setItem('theme', newMode)
        document.documentElement.classList.toggle('dark')
    }

    // Attach event listener
    document.getElementById('dark-toggle').addEventListener('click', toggleDarkMode)

    // Set initial theme
    const theme = localStorage.getItem('theme')
    if (theme === 'dark') {
        document.body.dataset.theme = theme
        document.documentElement.classList.toggle('dark')
    }
</script>

The weird thing is that if I delete the <DarkToggle /> component, the error dissapears, but for example each time I swap between posts, i get a very similar one! And yes! it has an inline script too. (to generate TOCs for each post, particularly)

VM2715:1  Uncaught SyntaxError: Identifier 'article' has already been declared
    at runScripts (ViewTransitions.astro:102:11)
    at navigate (ViewTransitions.astro:276:10)

The unique place in the whole codebase where i use article is [...slug].astro
And in fact, the error doesn't happen if I go from one of the posts to the home, it happens after i move to the another post

I'm assuming they are related ;P

@dawichi
Copy link

dawichi commented Sep 8, 2023

Fixed after moving both is:inline scripts to the same place where <ViewTransitions /> was used (in my case, layouts/Layout.astro as i dont use any BaseHead)

I tried to make it work on-component but its not an option apparently

@Alecyrus
Copy link

Alecyrus commented Sep 10, 2023

It is NOT werid.
#8482 (comment)

@matthewp matthewp removed the needs triage Issue needs to be triaged label Sep 19, 2023
@matthewp matthewp self-assigned this Sep 19, 2023
@matthewp matthewp added feat: view transitions Related to the View Transitions feature (scope) - P3: minor bug An edge case that only affects very specific usage (priority) labels Sep 19, 2023
@matthewp
Copy link
Contributor

I think the issue here is that if the script is within the body we do not prevent re-execution, but do so for scripts in the head. Should be easy enough to fix.

@matthewp matthewp added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Sep 19, 2023
@morja
Copy link

morja commented Jan 23, 2024

I am having the same issue now when using view transitions with Astro version 4.2.3.

It does not seem to matter if I use is:inline or not.

<script type="text/javascript" is:inline>
    let emt = document.getElementById('emt');
    ...
Uncaught SyntaxError: Identifier 'emt' has already been declared
    at runScripts (router.js?v=cefd6512:116:12)
    at router.js?v=cefd6512:376:11

@morja
Copy link

morja commented Jan 24, 2024

I think this is related to you using type="text/javascript", if you remove that I think the script wont reexecute @morja If you want to open a support ticket on discord and tag me we can work it out there

Thanks a lot @OliverSpeir , indeed this does the trick. It works with plain <script> but not with either type="text/javascript" or is:inline.

@martrapp
Copy link
Member

martrapp commented Jan 24, 2024

Thanks a lot @OliverSpeir , indeed this does the trick. It works with plain <script> but not with either type="text/javascript" or is:inline.

It is true that scripts without any properties aren't re-executed as they become module scripts referenced from the <head> which can by definition only reload on full page loads.

@morja, whatever your original issue was: scripts with is:inline or type="text/javascript" should not re-execute in v4.2.3 if they have identical attributes and bodies as seen before since the last full page load as on the previous page

Maybe we can investigate further on discord #support or you open another issue for your case with a minimal reproduction?

@OliverSpeir
Copy link
Contributor

For posterity sake I was wrong about type="text/javascript" having anything to do with this!

@martrapp
Copy link
Member

TIL: The check whether an inline script from the new page shouldn't be re-executed only compares against
scripts on the current page, not against all scripts executed since the last full page load.

@xArrietAx
Copy link

xArrietAx commented Jun 13, 2024

Hi, I imagine people here know how to fix the error, but for new visitors here is my solution.

When I use ViewTransitions component I don't get the scripts after going to another page.

Then I found this https://docs.astro.build/en/tutorials/add-view-transitions/

then I did this

<script>
  document.addEventListener('astro:page-load', () => { 
// Your code
  })
  </script>

I just wrapped my code with that event and it already works.

@martrapp
Copy link
Member

Hi @xArrietAx, thank you for sharing this and helping others!
This is the way to go!

Further information on how to rerun code after view transitions can also be found here

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: view transitions Related to the View Transitions feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants