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

feat: better CSS parsing #21978

Merged
merged 4 commits into from
Apr 30, 2024
Merged

feat: better CSS parsing #21978

merged 4 commits into from
Apr 30, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Apr 30, 2024

Problem

Towards #18520

Changes

  • This applies the changes suggested in feat: add new css parser - postcss rrweb-io/rrweb#1458 to handle CSS parsing better, while maintaining efficiency
  • General approach is to install postcss node module in posthog and reference code from within a patch applied to the rrweb package

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

  • Verified that playback works locally & that necessary functions are available
  • This feels like a somewhat risky change to me
    • What if the postcss is tree shaken in production and cannot be accessed 🤔
    • Is there any way to test that before shipping?

--- /dev/null
+++ b/es/rrweb/packages/rrweb-snapshot/es/css.js
@@ -0,0 +1,87 @@
+import postcss from '../../../../../../../../../postcss/lib/postcss.js'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directory is after the patch is applied e.g. node_modules/.pnpm/rrweb@2.0.0-alpha.13_patch_hash=6bgnomayryopibyfydha2z5wbi/node_modules/rrweb/es/rrweb/packages/rrweb-snapshot/es

- whitespace();
- while (css[0] == '}') {
- error('extra closing bracket');
- css = css.slice(1);
- whitespace();
- }
- const m = match(/^(("(?:\\"|[^"])*"|'(?:\\'|[^'])*'|[^{])+)/);
+ const m = match(/^([^{]+)/);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diff gets a little funky here because we are removing a previously applied patch

}

-const commentre = /\/\*[^*]*\*+([^/*][^*]*\*+)*\//g;
-function parse(css, options = {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes all the old ast generation code because of a naming clash

@daibhin daibhin requested a review from a team April 30, 2024 13:31
@marandaneto
Copy link
Member

This is huge, hopefully its a smooth patch 😅

@daibhin daibhin force-pushed the dn-feat/better-css-parsing branch from 87a5c0b to a756743 Compare April 30, 2024 15:53
@daibhin
Copy link
Contributor Author

daibhin commented Apr 30, 2024

@marandaneto I got scared 🙈 added a try / catch with the existing code that it should fall back to in production. Going to ship once 🍏

@marandaneto
Copy link
Member

that

Very clever 🚀

@daibhin daibhin merged commit 9f65672 into master Apr 30, 2024
96 checks passed
@daibhin daibhin deleted the dn-feat/better-css-parsing branch April 30, 2024 16:25
@daibhin
Copy link
Contributor Author

daibhin commented Apr 30, 2024

Change is live and everything works as expected 🙌

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.

2 participants