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

define:vars not working in Safari #5064

Closed
1 task done
aleksa-codes opened this issue Oct 13, 2022 · 12 comments · Fixed by #5243
Closed
1 task done

define:vars not working in Safari #5064

aleksa-codes opened this issue Oct 13, 2022 · 12 comments · Fixed by #5243
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) - P5: urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority)

Comments

@aleksa-codes
Copy link
Contributor

aleksa-codes commented Oct 13, 2022

What version of astro are you using?

1.4.6

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

None

What package manager are you using?

yarn

What operating system are you using?

Mac, iOS

Describe the Bug

The issue was closed but the problem is still there: #4859

Check my footer in Safari on Mac/iOS for a reproduction of the problem.

code: https://github.com/aleksa-codes/astro-portfolio/blob/main/src/components/Footer.astro

Link to Minimal Reproducible Example

https://aleksa.codes

Participation

  • I am willing to submit a pull request for this issue.
@rhizodigital
Copy link

I have same issue, In scripts the value is available directly inside script tag, but not inside a function. Works in firefox and chrome.

Screenshot 2022-10-13 at 10 00 25

@matthewp matthewp added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Oct 13, 2022
@rishi-raj-jain
Copy link
Contributor

@aleksa-codes @matthewp something is making <script> as <script>{ when being added as a HTML Parts before render takes place. Not able to find the place where this extra curly brace could've been added.

Screenshot 2022-10-14 at 11 37 32 AM

@rishi-raj-jain
Copy link
Contributor

@aleksa-codes I think it's more of a astro/compiler issue to detect? Those extra curly braces after the tag 👀

@janreges
Copy link

+1 ... curly braces {} which Astro adds to <script> with define:vars also causes the Alpine JS scripts to malfunction in Safari.

@matthewp is there any chance a fix for this critical problem will be in the next release of Astro?

@janreges
Copy link

janreges commented Oct 25, 2022

@matthewp and @natemoo-re - all these problems in Safari are probably caused by changes in this PR #3976

I understand that {} are used to avoid redeclaration errors (which can be encountered by some use-cases), but unfortunately it causes fatal problems in Safari.

Unless there is a better solution for this situation, I suggest that the addition of {} be controlled by some configuration parameter. What do you think about it?

At the same time, I think that if someone decides to use define:vars in a repeatedly inserted component, they should ensure that these global variables have unique names. Typically containing e.g. entity ID, so e.g. <script define:vars={{`name_${componentId}`: $name}}`> ... </script>

@rishi-raj-jain
Copy link
Contributor

rishi-raj-jain commented Oct 26, 2022 via email

@janreges
Copy link

@rishi-raj-jain - it was a good intention from @natemoo-re and it solves a situation where a component with <script define:vars> is inserted more than once on the page. Thanks to that, wrapping it in {} will not cause reassignment, which would otherwise result in an error.

In my opinion, the addition of {} should not be automatic, but should be set by explicitly setting, for example, is:scoped. So {} would only be added in the case of a definition e.g. <script is:scoped define:vars={{ ... }}>.

At the same time, I would expect this behavior to be implemented in the renderElement method:

export function renderElement(

I was debugging this last night and it was very confusing to me that the logic isn't just inside the method.

@maxcountryman
Copy link

Hi folks, are there any workarounds in the meantime? I can rewrite component with React, etc but that is truly an awful situation to be in. Safari is not a niche browser, so this feels like an incredibly important issue to address.

@matthewp
Copy link
Contributor

@janreges do you know the reason why the {} breaks in safari? Agree that this is more of a priority.

@matthewp matthewp added the - P5: urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority) label Oct 28, 2022
@matthewp
Copy link
Contributor

Pretty sure it's this bug: https://bugs.webkit.org/show_bug.cgi?id=179698

@matthewp
Copy link
Contributor

Got a fix in the works.

@matthewp
Copy link
Contributor

Fix is in withastro/compiler#599

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) - P5: urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants