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

Fix Safari runtime error for..of weirdness #55

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Fix Safari runtime error for..of weirdness #55

merged 2 commits into from
Mar 29, 2022

Conversation

luwes
Copy link
Contributor

@luwes luwes commented Mar 25, 2022

We've been using template-parts in our new video player project over at Mux.
We really like it with the added utilities of jtml!

Today we ran into a rather strange runtime error only in Safari and it only happens when the web inspector is not open.
Try loading this page with and without web inspector open, might need a few reloads:
https://elements-demo-nextjs-git-fork-luwes-better-errors-mux.vercel.app/MuxPlayer

  1. Without web inspector open, the video player layout sometimes doesn't look good, the layout of icons and buttons are wrong.
  2. If you then open the web inspector you see a type error like
TypeError: e of xr is not a function. (In 'e of xr(t.textContent)', 'e of xr' is undefined)

This refers to this line:
https://github.com/github/template-parts/blob/main/src/template-instance.ts#L28

I don't think the code is wrong here but Safari definitely misinterprets it somehow.
Changing the for..of loop to a regular for loop fixes the issue,
I also tried to store the result of the parse() function in a variable first and use the for..of loop but that didn't fix it :(

SCR-20220325-d8k

SCR-20220325-dye

Mac OS 12.3
Safari 15.4

@luwes luwes requested a review from a team as a code owner March 25, 2022 20:06
@luwes luwes requested a review from theinterned March 25, 2022 20:06
@luwes
Copy link
Contributor Author

luwes commented Mar 26, 2022

mobile Safari on iOS 12 and probably other versions also seems to be affected
also seeing the console output here it shows it's throwing in native code and related to the generator, seeing generatorResume there. very much looks like a browser bug unfortunately.

SCR-20220325-srl

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

That's super weird! Thanks for the fix!

for (const token of parse(node.textContent)) {
const parsed = parse(node.textContent)
for (let i = 0; i < parsed.length; i += 1) {
const token = parsed[i]

Choose a reason for hiding this comment

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

Small comment: Do you think a code comment with a link to this PR for context would be helpful?

I'd be a little worried that someone sees this code and refactors it back to for const ... in, resulting in a regression.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a great idea!

I'll merge this once a comment is added 😝

@dylanjha
Copy link

@keithamus I think this is ready now 🙌🏼 thanks for your help!

@keithamus keithamus merged commit 75018f0 into github:main Mar 29, 2022
@dylanjha
Copy link

Will a new version be coming out soon :) we're currently pointed to a fork to get this patch, but we would love to point back upstream when this is released!

@cjpillsbury
Copy link

Hey @keithamus just checking in to see if there were any plans to do an npm release that includes this update.

@keithamus
Copy link
Member

Oh sorry! This one slipped by 😬. Should be out as v0.5.2

@cjpillsbury
Copy link

No worries @keithamus ! Just a heads up - looks like there were some pathing mismatches with your latest publish. I'll be sure to throw an issue up (and may be able to help get it fixed, time permitting), but here's the short of it:

  • main and module point to lib/index.js and files also includes lib
  • lib/index.js doesn't exist, but lib/src/index.js exists (as does lib/test/*, so I'm suspecting it was a build destination pathing issue?)

Workaround:
https://github.com/muxinc/elements/blob/main/packages/mux-player/src/html.ts#L1-L9

@keithamus
Copy link
Member

Oh no 🙈. We seem to be having some publish difficulties. I'll make sure to get these resolved soon!

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.

4 participants