Guard against matchMedia access during initial render #1459

somewhatabstract · 2024-07-29T21:47:56Z

Summary:

This PR makes sure that we don't call matchMedia during the initial render. This is because matchMedia is not available during the initial render, and calling it will throw an error.

Issue: FEI-5743

Test plan:

Put up this PR and await the pre-release packages
Update webapp with the pre-release packages
Put up a webapp PR and check that the ZND is working on URLs that were previously erroring

khan-actions-bot · 2024-07-29T21:48:10Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/quiet-moose-arrive.md, packages/perseus/src/widgets/explanation.tsx, packages/perseus/src/widgets/label-image.tsx, packages/perseus/src/widgets/label-image/marker.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-07-29T21:49:42Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (cea126a) and published it to npm. You
can install it using the tag PR1459.

Example:

yarn add @khanacademy/perseus@PR1459

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1459

somewhatabstract · 2024-07-29T21:48:48Z

packages/perseus/src/widgets/explanation.tsx

+            this._mounted &&
+            mediaQueryIsMatched("(prefers-reduced-motion: no-preference)");


note: We don't want animation on by default on initial render in case the user has turned it off (long term, it would be useful if this also could be overridden by webapp profile setting too).

I'm confused how this fixes things. The mediaQueryIsMatched function guards against window.matchMedia not being a function. Is that helper written wrong or is the fix more about only running on a mounted component?

This is a subtle thing, but we don't just want to guard against it being accessed; the initial render on both server and client must be identical, so we don't want to use matchMedia at all on first render as the client could give a different rendered result than the server.

somewhatabstract · 2024-07-29T21:49:14Z

packages/perseus/src/widgets/label-image/marker.tsx

@@ -106,7 +119,7 @@ export default class Marker extends React.Component<Props> {
        } else if (showPulsate) {
            iconStyles = [
                styles.markerPulsateBase,
-                shouldReduceMotion()
+                this._mounted && shouldReduceMotion()


note: We don't want animation on by default on initial render in case the user has turned it off (long term, it would be useful if this also could be overridden by webapp profile setting too).

somewhatabstract · 2024-07-29T21:49:48Z

packages/perseus/src/widgets/label-image.tsx

-            const isWideImage =
-                this.props.imageWidth / 2 > this.props.imageHeight;
+        // Determine whether the image is wider than it is tall.
+        const isWideImage = this.props.imageWidth / 2 > this.props.imageHeight;


note: Moved out of the loop; doesn't need recalculating multiple times.

Thanks for fixing this up!

somewhatabstract · 2024-07-29T21:50:22Z

packages/perseus/src/widgets/label-image.tsx

+            this._mounted &&
+            window.matchMedia(mediaQueries.xsOrSmaller.replace("@media ", ""))
+                .matches;


note: Moved out of the loop since it won't change for each item, and modified to return false on initial render so that it will SSR and hydrate correctly.

github-actions · 2024-07-29T21:50:43Z

Size Change: +86 B (+0.01%)

Total Size: 849 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	411 kB	+86 B (+0.02%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.2 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.6 kB
`packages/math-input/dist/es/strings.js`	1.73 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	270 kB
`packages/perseus-linter/dist/es/index.js`	21.6 kB
`packages/perseus/dist/es/strings.js`	3.21 kB
`packages/pure-markdown/dist/es/index.js`	3.67 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-07-29T21:53:47Z

Codecov Report

Attention: Patch coverage is 79.54545% with 9 lines in your changes missing coverage. Please review.

Project coverage is 70.12%. Comparing base (9bc4812) to head (cea126a).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1459      +/-   ##
==========================================
+ Coverage   69.79%   70.12%   +0.33%     
==========================================
  Files         505      508       +3     
  Lines      105003   104905      -98     
  Branches     7548    10713    +3165     
==========================================
+ Hits        73283    73565     +282     
+ Misses      31536    31340     -196     
+ Partials      184        0     -184

Files	Coverage Δ
packages/perseus/src/widgets/explanation.tsx	`93.44% <91.66%> (+1.52%)`	⬆️
packages/perseus/src/widgets/label-image.tsx	`83.11% <83.33%> (-0.06%)`	⬇️
...ackages/perseus/src/widgets/label-image/marker.tsx	`90.24% <64.28%> (-1.37%)`	⬇️

... and 151 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bc4812...cea126a. Read the comment docs.

jeresig

These changes all seem reasonable to me, thank you for figuring this out, Jeff! I'll defer to the Perseus team for final approval here, though.

jeremywiebe

The fixes mostly make sense. I left one question and I think it would make sense to move the mediaQueryIsMatched helper to a new file in packages/perseus/src/util. Not a deal-breaker, but if you decide not to please let me know and I'll create a ticket to do this after the fact.

Thanks for chasing this down Jeff.

jeremywiebe · 2024-07-30T14:47:01Z

packages/perseus/src/widgets/explanation.tsx

+            this._mounted &&
+            mediaQueryIsMatched("(prefers-reduced-motion: no-preference)");


I'm confused how this fixes things. The mediaQueryIsMatched function guards against window.matchMedia not being a function. Is that helper written wrong or is the fix more about only running on a mounted component?

jeremywiebe · 2024-07-30T14:48:05Z

packages/perseus/src/widgets/label-image.tsx

+        // Determine whether page is rendered in a narrow browser window.
+        const isNarrowPage =
+            this._mounted &&
+            window.matchMedia(mediaQueries.xsOrSmaller.replace("@media ", ""))


This usage doesn't use a helper to guard window.matchMedia being undefined. I wonder if we should move all these checks to call a helper that guards usage properly. Would that alleviate us needing the _mounted flag in each component?

Guarding against it being undefined is really a red herring; I think perhaps that check should be removed where we have it. I don't think we support any browsers that won't have it at this point, and we shouldn't be determining by that measure if we call it or not on initial render as we need that to match on both client (where it exists) and server (where it doesn't exist or might give a different response than on the client).

The _mounted flag is the important bit to avoid having a different initial render on the client than on the server (both initial renders have to match or hydration fails)

Guard against matchMedia access during initial render

1c5d7fd

somewhatabstract self-assigned this Jul 29, 2024

somewhatabstract requested review from a team July 29, 2024 21:47

somewhatabstract commented Jul 29, 2024

View reviewed changes

Add changeset

cea126a

jeresig approved these changes Jul 30, 2024

View reviewed changes

jeremywiebe approved these changes Jul 30, 2024

View reviewed changes

somewhatabstract merged commit be40d77 into main Jul 30, 2024
11 checks passed

somewhatabstract deleted the matchMediaFix branch July 30, 2024 15:33

khan-actions-bot mentioned this pull request Jul 30, 2024

Version Packages #1460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard against matchMedia access during initial render #1459

Guard against matchMedia access during initial render #1459

somewhatabstract commented Jul 29, 2024

khan-actions-bot commented Jul 29, 2024 •

edited

Loading

github-actions bot commented Jul 29, 2024 •

edited

Loading

somewhatabstract Jul 29, 2024

jeremywiebe Jul 30, 2024

somewhatabstract Jul 30, 2024

somewhatabstract Jul 29, 2024

somewhatabstract Jul 29, 2024

jeremywiebe Jul 30, 2024

somewhatabstract Jul 29, 2024

github-actions bot commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

jeresig left a comment

jeremywiebe left a comment

jeremywiebe Jul 30, 2024

jeremywiebe Jul 30, 2024

somewhatabstract Jul 30, 2024 •

edited

Loading

		this._mounted &&
		mediaQueryIsMatched("(prefers-reduced-motion: no-preference)");

Guard against matchMedia access during initial render #1459

Guard against matchMedia access during initial render #1459

Conversation

somewhatabstract commented Jul 29, 2024

Summary:

Test plan:

khan-actions-bot commented Jul 29, 2024 • edited Loading

Gerald

github-actions bot commented Jul 29, 2024 • edited Loading

npm Snapshot: Published

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 29, 2024 • edited Loading

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

jeresig left a comment

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

khan-actions-bot commented Jul 29, 2024 •

edited

Loading

github-actions bot commented Jul 29, 2024 •

edited

Loading

github-actions bot commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

somewhatabstract Jul 30, 2024 •

edited

Loading