In case this is false, couldn't we stop the loop? Since there's no way settings.supports.everything will be true if the first execution of the loop results in settings.supports.everything = false.

At first glance that seems to be correct, but it seems we need to keep looping over everything because we need to set everythingExceptFlag properly. At second glance, this doesn't also seem to be needed, but I did a quick ack and found it is used in emoji.js:

So we still do need to loop over everything, I believe.

This code is problematic since the function name will change when the code is minimized.

Oh, that's a great point. Thanks for catching that.

Should be fixed with 44c09ee

I'm getting an error here:

Running "jsvalidate:build" (jsvalidate) task
Validating build/wp-includes/js/wp-emoji-loader.js  
>> Line 22: Unexpected token function

But I'm not immediately clear why.

It may be due to Esprima not supporting ECMAScript 2017 yet? Or maybe there's a way to enable support in the esprimaOptions config:

I haven't been able to quickly determine what the possible options are.

The version of grunt-jsvalidate used in core is 0.2.2, which is the latest version available. It was last updated 10 years ago. What's more is that the GitHub repo has been archived. So clearly we need to use something else to check for syntax errors in JS files.

I responded to your question about this in slack, but it may also be time for us to consider removing this validation step entirely at this point. Seems like perhaps this old validation task is causing more friction in our build process than it is helping.

For the sake of expediency for beta1, I'm going to try to make this ES3-compatible and then we can revisit taking advantage of ES6+.

To account for users who never close their browser/constantly duplicate tabs of their CMS, it it worth including a hash (or similar) of the values been passed to emojiSetsRenderIdentically? A time stamp might be an alternative to retest every 24-48 hours.

I don't think that's necessary because presumably any change to the platform emoji support would require a reboot, and that would result in a new session, correct?

Seems that may not be correct. If rebooting and browser tabs are restored, then the sessionStorage would need to persist.

I've made this change in d694445

The data stored in sessionStorage is only considered if it isn't older than a day.

this is also not a big deal, but it's curious that we interrupt the sessionSupportsPromise to create the domReadyPromise here. if we put domReadyPromise above sessionSupportsPromise couldn't we eliminate the creation of a variable for sessionSupportsPromise entirely and leave it as a single continuous chain instead of trapping a promise and then "re-starting" it in the code later?

var domReadyPromise = new Promise( … );

new Promise( function ( resolve ) {
	…
} )
	.then( function ( sessionSupports ) {

	} )
	.then( function () {
		return domReadyPromise;
	} )
	.then( function () {
		…
	} );

is there intentionally no error-handling here? if any of the Promise callbacks throw then nothing happens and settings aren't updated.

would it be worth adding a final "global" .catch() as a catch-all "we couldn't determine support so support isn't guaranteed"?

this is also not a big deal, but it's curious that we interrupt the sessionSupportsPromise to create the domReadyPromise here.

I thought maybe it was easier to read, but apparently not. So I've made the change in aaf5718

is there intentionally no error-handling here? if any of the Promise callbacks throw then nothing happens and settings aren't updated.

would it be worth adding a final "global" .catch() as a catch-all "we couldn't determine support so support isn't guaranteed"?

There are try/catch blocks around code that I expect might throw errors. In the case where an error does occur, the result would be Twemoji wouldn't be loaded. I don't know if there should be a different outcome?

There are try/catch blocks around code that I expect might throw errors.

makes sense. I guess we have to trust that the code inside those Promises won't throw. if for some reason they would (such as the unexpected bug with invalid code points) we would get an error about an unhandled catch, I think.

I don't know if there should be a different outcome?

maybe the only outcome is swallowing the errors and doing any necessary cleanup. if leaving session storage untouched is all the cleanup that's required, that makes sense. I have assumed that the failure mode is to make sure that twemoji and other scripts load, if we cannot determine that they aren't needed.

The current emoji loader logic in trunk doesn't have any such error handling to force Twemoji to load in the case of an exception while gathering settings. I think we can leave that for future consideration.

this is ultimately tied to the version of Twemoji that WordPress ships isn't it? I think that the fallback case we're discussing is somewhat benign: if someone reboots and re-opens tabs and now their OS adds additional support for Unicode 14 Emoji, the worst thing that happens is that we continue to replace them until those tabs or the session storage clears. That's not that bad.

With this change we are going back to computing this at least once per day, which for many sites could very well be every page load.

Is the risky scenario that we update Twemoji in Core and can handle the next set of Emoji additions, but because this script cached the results it doesn't replace them and then the page is rendered improperly?

In that case could we build in some sentinel indicating the Twemoji version and then handle this deterministically? Update automatically when Twemoji updates or in the natural flow of browser reloads? Otherwise be okay with being a bit too cautious for some amount of time after Unicode releases?

I think that the fallback case we're discussing is somewhat benign: if someone reboots and re-opens tabs and now their OS adds additional support for Unicode 14 Emoji, the worst thing that happens is that we continue to replace them until those tabs or the session storage clears. That's not that bad.

Correct.

this is ultimately tied to the version of Twemoji that WordPress ships isn't it?

I don't think it's tied to the version of Twemoji but rather to whatever tests are present in browserSupportsEmoji(), which are [ 'flag', 'emoji' ]. I'm not familiar with the history of updating that function with new tests. Are you saying that instead of testing for a timestamp we should check to see if all of the tests are present? So if core adds another test we invalidate the cache to go ahead and compute the new results?

cc @peterwilsoncc

we should check to see if all of the tests are present?

actually this would be way better than what I suggested and wouldn't need to have any coupling with Twemoji. that's a brilliant idea, because the tests are trivial to serialize.

it wouldn't just be the names, but rather the specific strings we compare, or at least one of the two in comparison.

the ultimate goal is to use some canary string that we know is only supported in some version of the Unicode release. if that canary isn't present then we need Twemoji to fill in the gap.

if we don't add the test for it, the version of Twemoji wouldn't matter because we wouldn't be queuing it up anyway. supposing that Unicode releases a new version of the Emoji and a new character is available, we would need to add a new test to check if it renders as we expect. until then, the tests here would remain the same even though the rendering might be broken.

If the session storage cache is varied by the strings we compare, I don't think we can easily do so without computing a hash of the browserSupportsEmoji function and using this string for comparison. Perhaps a more efficient means would be to just introduce a emojiLoaderVersion that we bump whenever we touch the browserSupportsEmoji function to modify the strings?

Well, the cache should be invalidated whenever test strings change (when browserSupportsEmoji() is changed) or the OS emoji support changes. I think going ahead with just the 1 day expiration for now is adequate. We can consider further optimization after beta1.

I did so in b27e713.

So you'd be happier with a week cache?

I'm happy with all you've done in this PR no matter how you choose the timeout. 😄

@westonruter @dmsnell Sorry I didn't do a better job of explaining my thinking.

The thought behind the comment was indeed related to the twemoji (or WP version). I was thinking of the following situation:

• A user opens the browser, sessionStorage is set
• The user leaves the browser open overnight
• The site admins update WP to a version that includes an update to twemoji and the related sniffs
• The user returns and continues in the existing tab, as they navigate WP scripts will use the new version
• Without an expiration control the sniff would continue to be based on the previous tests.

Dennis, you raise a good point that the stale check could be based on the twemoji or WP version. That would allow the sessionStorage to remain valid until a potential change is detected

The origin of this thought was when I worked for a news organisation and discovered that journos keep their browsers open forever, constantly duplicating tabs. The team I was on was using both session and local storage and we had to add some code to invalidate it upon a deploy. Several years later, this experience continues to be traumatic. :)

Several years later, this experience continues to be traumatic. :)

That's understandable. Also why I like finding ways to make durable cache keys (for instance, "properly renders Emoji added in Unicode 12"

Been burnt the other way too, with "we'll only leave in daily expiration until the project stablizes…five years later" 😆

It's like cache invalidation is hard or something.

Honestly I'd love to look into redoing this after @westonruter suggested storing the test names as the key. Twemoji incorporates what looks like a 13 KB regular expression pattern that includes the entire database of replacements up through Unicode 14 (curious if and when they update to Unicode 15). If these were separated out we could potentially cut out 10 KB+ of download when required, and our session cache would remain durable under all but the most extreme edge cases (someone downgrades their operating system and re-opens a browser on the same tabs and is also not okay with missing Emoji)

I've opened a Trac ticket we can use to track this going forward: Core-58663. I've marked it very low priority.