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

misc: avoid splitting node labels in the middle of unicode surrogate pairs #11698

Merged
merged 12 commits into from
Dec 6, 2020
Merged

misc: avoid splitting node labels in the middle of unicode surrogate pairs #11698

merged 12 commits into from
Dec 6, 2020

Conversation

wildlyinaccurate
Copy link
Contributor

@wildlyinaccurate wildlyinaccurate commented Nov 23, 2020

Summary

Potential fix for #11697. Takes advantage of the fact that the ES6 string iterator is unicode-aware.

Related Issues/PRs

fix for part of #11697

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks very much @wildlyinaccurate! 🎉

great find, PR much appreciated :)

lighthouse-core/lib/page-functions.js Show resolved Hide resolved
@wildlyinaccurate
Copy link
Contributor Author

Thanks for the suggestions @patrickhulce - much appreciated!

@wildlyinaccurate wildlyinaccurate changed the title Avoid splitting node labels in the middle of unicode surrogate pairs misc: Avoid splitting node labels in the middle of unicode surrogate pairs Nov 23, 2020
@wildlyinaccurate wildlyinaccurate changed the title misc: Avoid splitting node labels in the middle of unicode surrogate pairs misc: avoid splitting node labels in the middle of unicode surrogate pairs Nov 23, 2020
@wildlyinaccurate
Copy link
Contributor Author

yarn update:test-devtools is failing on my machine. I'm not sure if I need to manually update some of the expected output fixtures or not?

@patrickhulce
Copy link
Collaborator

It's actually failing on master too @wildlyinaccurate , so not your doing. We'll let this chill for a bit until we're back up and running 👍

@connorjclark
Copy link
Collaborator

master on my machine:
image

@patrickhulce
Copy link
Collaborator

@connorjclark
Copy link
Collaborator

Just landed #11702 to fix master.

How is yarn update:test-devtools failing, can you share the output? FYI it only works on Mac at the moment.

@patrickhulce
Copy link
Collaborator

FWIW I don't think you'll need to run yarn update:test-devtools once this is rebased though

@connorjclark
Copy link
Collaborator

I think the other 2 usages of .slice here are susceptible to the same issue. Hard to analyze the entire codebase tho, would be nice if one could search for [String].slice using the typescript parser.

@patrickhulce
Copy link
Collaborator

I think the other 2 usages of .slice here are susceptible to the same issue. Hard to analyze the entire codebase tho, would be nice if one could search for [String].slice using the typescript parser.

There are many usages of .slice across LH that are susceptible to this issue. How do you feel about just leaving open #11697 for investigation by the team but still landing this PR?

@google-cla
Copy link

google-cla bot commented Nov 24, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@@ -20,6 +20,13 @@ set -euxo pipefail

# We can always use some more history
git -c protocol.version=2 fetch --deepen=100

# Find out if the PR is coming from a fork
Copy link
Collaborator

Choose a reason for hiding this comment

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

@paulirish how do you feel about this skipping this check entirely if it's from a fork for now instead and we can sort out the fix later?

Copy link
Member

Choose a reason for hiding this comment

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

that works for me.. though determining that would probably have required almost this much iteration as well... so i think it's all moot.

ALL MOOT!

.github/scripts/git-get-shared-history.sh Outdated Show resolved Hide resolved
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
@wildlyinaccurate
Copy link
Contributor Author

Hey, uh, does it help if I resubmit this against master as a single commit?

@paulirish
Copy link
Member

Hey, uh, does it help if I resubmit this against master as a single commit?

haha nah man it's all good. we'll just all pretend I never had to make all that noise. the bug i had is fixed (as of my last commit), but it ends up not mattering.

image

This should be good to go once the yarn install flake timeout is gone.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

@wildlyinaccurate btw this is a fantastic find and fix. Nice work. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants