-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: Handle exceptions thrown from postcss when calling adaptCssForReplay #1580
base: master
Are you sure you want to change the base?
fix: Handle exceptions thrown from postcss when calling adaptCssForReplay #1580
Conversation
🦋 Changeset detectedLatest commit: 27f01fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
heads up, i think the root cause is #1575 |
@otan Good call out! That definitely seems like one of the ways invalid CSS can be passed to For this particular PR, I just wanted to simply make sure exceptions from |
## Summary Bring back postcss css parsing which offers better reliability with inlined stylesheets and better performance. Fixes replay of a session where the existing CSS parsing of rrweb was broken. Incorporates rrweb-io/rrweb#1580 to fix broken replays due to postcss crash. ## How did you test this change? https://preview.highlight.io/102751/sessions/35zPFX0OPXs166n5L7m1GEZaqWgL?%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D fixes replay that is otherwise broken https://preview.highlight.io/14083/sessions/77X2eTJA5TIbEydiUytAFmZhJoXn?commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&page=1&query=viewed_by_anyone%3Dtrue+visited-url%3D%22*6532da*%22&commentId=90302&commentId=90302&%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D ## Are there any deployment considerations? frontend only continues to work ![Screenshot from 2024-10-09 17-36-41](https://github.com/user-attachments/assets/f4e6b68a-e993-4292-9d76-0049d3e7b797) ## Does this work require review from our design team? no
a08c325
to
ae97dac
Compare
- Apply formatting changes - Remove import of postcss in test file Upstream PR: rrweb-io#1580
ae97dac
to
c1accef
Compare
- Apply formatting changes - Remove import of postcss in test file
c1accef
to
27f01fe
Compare
## Summary Bring back postcss css parsing which offers better reliability with inlined stylesheets and better performance. Fixes replay of a session where the existing CSS parsing of rrweb was broken. Incorporates rrweb-io/rrweb#1580 to fix broken replays due to postcss crash. ## How did you test this change? https://preview.highlight.io/102751/sessions/35zPFX0OPXs166n5L7m1GEZaqWgL?%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D fixes replay that is otherwise broken https://preview.highlight.io/14083/sessions/77X2eTJA5TIbEydiUytAFmZhJoXn?commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&commentId=90302&page=1&query=viewed_by_anyone%3Dtrue+visited-url%3D%22*6532da*%22&commentId=90302&commentId=90302&%7Er_preview=%7B%22action%22%3A%22start%22%2C%22mode%22%3A%22production%22%2C%22region%22%3A%22ewr%22%2C%22variantId%22%3A%2201J8GFGDTWBWCRZZZ7G92P9HJX%22%2C%22variantData%22%3A%22%7B%5C%22type%5C%22%3A%5C%22branch%5C%22%2C%5C%22branch%5C%22%3A%5C%22vadim%2Fsession-troubleshooting%5C%22%2C%5C%22githubOwnerName%5C%22%3A%5C%22highlight%5C%22%2C%5C%22githubRepositoryName%5C%22%3A%5C%22highlight%5C%22%7D%22%7D ## Are there any deployment considerations? frontend only continues to work ![Screenshot from 2024-10-09 17-36-41](https://github.com/user-attachments/assets/f4e6b68a-e993-4292-9d76-0049d3e7b797) ## Does this work require review from our design team? no
@eoghanmurray thanks for taking a look at my PR! I'll need to double check the original issue I was encountering to see if #1600 fixes it (I thought I checked and it didn't but I honestly can't remember 😓). In any case, I am inclined to keep the try/catch just to prevent any future exceptions from postcss from going unhandled. I'm happy to make any updates to how the exceptions are logged (or any other issue you see). Thanks again for the review! |
This PR adds a
try/catch
aroundpostcss().process()
so that exceptions won't cause playback to crash. I was encountering issues where invalid CSS was being passed topostcss
which would result in CssSyntaxError. I am still investigating why invalid CSS is passed to adaptCssForReplay in the first place, but for now this change should at least make sure any exceptions frompostcss
are at least handled and logged. I am currently usingwarn
to log the exceptions but am happy to switch that to something else if need be.Related discussion in rrweb slack: https://rrweb.slack.com/archives/C01BYDC5C93/p1727975312250899