Truncate and map filenames that are too long in test archiver #24

jwir3 · 2023-10-10T16:08:09Z

Issue: CAP-1203

What Changed

Sometimes, filenames are too long to be written to the file system. Typically, this happens if a file name is longer than 256 bytes. When this happens, we want to truncate the file name and map references to this file to the new, shortened filename.

How to test

There are tests for this in write-archive/index.test.ts, but you can follow these steps to reproduce:

Link this branch so that you can use test-archiver locally or install version @chromaui/test-archiver@0.0.27--canary.24.9cb64d2.0 of the package using: yarn add -D @chromaui/test-archiver@0.0.27--canary.24.9cb64d2.0.
Clone https://github.com/tevanoff/chromatic-e2e-issues
Run yarn playwright test toolong.test.ts
Verify that no error occurs.

Change Type

patch

📦 Published PR as canary version: 0.0.27--canary.24.5750afe.0

✨ Test out this PR locally via:

npm install @chromaui/test-archiver@0.0.27--canary.24.5750afe.0
# or 
yarn add @chromaui/test-archiver@0.0.27--canary.24.5750afe.0

This extracts the utility functions for comparing archives into a separate file, testUtils.

linear · 2023-10-10T16:08:13Z

CAP-1203 ENAMETOOLONG error during archiving

Filenames that are too long for the OS (or node?) will fail when archiving.

To replicate:

Clone https://github.com/tevanoff/chromatic-e2e-issues
Run yarn playwright test toolong.test.ts
Then:

$ yarn playwright test toolong.test.ts

...
Error: ENAMETOOLONG: name too long, open ...

src/write-archive/index.test.ts

skitterm

Some nitpicks, but nice work on truncating across rrweb!

package.json

src/playwright-api/makeTest.ts

src/playwright-api/takeArchive.ts

src/write-archive/index.test.ts

src/write-archive/index.ts

tmeasday

Can you talk through the decision to alter the result of rrweb rather than modifying rrweb itself to do the rewriting?

Is this something we've decided in general (that we'll make our changes in a second pass over its JSON output)?

src/write-archive/index.ts

src/write-archive/index.test.ts

jwir3 · 2023-10-11T14:07:58Z

Can you talk through the decision to alter the result of rrweb rather than modifying rrweb itself to do the rewriting?

Is this something we've decided in general (that we'll make our changes in a second pass over its JSON output)?

Sure! I was under the impression, based on talks that we've had previously, that rrweb is likely going to go away in the near future, towards an integrated solution within our own code. This begins to move toward that, although, I do admit that I probably should've verified with the team before making this decision.

Further, this case (mapping a specific link from one thing to another like this), seems to be a pretty specialized case within our own code, rather than a problem that will occur within rrweb itself. I'm hesitant to move our fork of rrweb-snapshot further from the upstream, if possible, especially to satisfy a niche that we have in our application.

I'd be willing to migrate this to rrweb-snapshot, if so desired, if the team thinks it's a cleaner solution to add this as a feature to that codebase instead of our application. I think we'll probably have to align on whether we're going to be maintaining the fork of rrweb-snapshot and using in in the long-term, though. Perhaps we should have a zoom meeting to discuss this?

Sometimes, if a filename is too long (over 256 bytes, usually), then test-archiver cannot write to most file systems. We need to truncate filenames that are longer than this and then map the references to these file names to the shorter version. Fixes CAP-1203.

tevanoff · 2023-10-11T17:33:07Z

src/playwright-api/takeArchive.ts

+  existingSourceMap: Map<string, string>
+): Map<string, string> {
+  // eslint-disable-next-line no-restricted-syntax
+  for (const nextChildNode of input) {


It looks like we're walking the entire dom tree twice -- once here and once in write-archive. Could we possible get that down to once? Maybe by catching the file name errors when trying to write them to disk rather than looking at the length in this pre-processing step?

Hm, I'm not sure we're going to be able to condense it into a single walk. We need the source mappings before we start writing to files, otherwise we won't have the data we need when we go to perform the rewrite as we output the files.

tevanoff · 2023-10-11T17:36:24Z

src/playwright-api/takeArchive.ts

@@ -36,12 +42,47 @@ async function takeArchive(
  });

  // Serialize and capture the DOM
-  const domSnapshot: serializedNode = await page.evaluate(dedent`
+  const domSnapshot: elementNode = await page.evaluate(dedent`


What about assets loaded through css? Are those represented in rrweb's dom tree?

Oh, that's a good question. I'll create a test case and verify that they are being loaded.

Curious if you were able to verify this, although I don't think we need to block this PR on it in any case.

I wasn't able to verify one way or the other. After doing some research on rr-web itself, though, for our potential rewrite project, I got the distinct impression that rr-web serializes assets loaded from CSS. However, I wasn't able to get a test case put together that exhibits this behavior on a CSS asset that would have too long of a filename. Once this PR is merged, I can go back and look at it again and see if I can get a test case put together.

tmeasday · 2023-10-11T22:53:51Z

Perhaps we should have a zoom meeting to discuss this?

Agree we should get alignment on this. I'm not necessarily opposed to this approach -- one advantage of rrweb's JSON format is we can do transformations like this to it fairly easily. I wonder if we could switch back to using a vanilla version of it and use a similar post-processing step to achieve the changes I made to it (I'm thinking probably not but I figured I'd ask).

jwir3 · 2023-10-13T16:09:23Z

Perhaps we should have a zoom meeting to discuss this?

Agree we should get alignment on this. I'm not necessarily opposed to this approach -- one advantage of rrweb's JSON format is we can do transformations like this to it fairly easily. I wonder if we could switch back to using a vanilla version of it and use a similar post-processing step to achieve the changes I made to it (I'm thinking probably not but I figured I'd ask).

After speaking with @thafryer, his opinion is we should move forward with this solution, since it unblocks CAP-1204 and CAP-1124. If we decide that we're going to remove rrweb, or, even move this code into rrweb, then we can do that in a followup commit. (This is just for convenience, since @tmeasday you're out this week and most of next week). I'm happy to schedule a meeting once you're back.

tevanoff · 2023-10-16T15:56:46Z

package.json

@@ -11,13 +11,6 @@
  "main": "dist/index.js",
  "module": "dist/index.mjs",
  "types": "dist/index.d.ts",
-  "exports": {


Did you mean to delete all this?

Hm... I did not. I only meant to remove src/ from the exports. I'll fix this and the test:playwright scripts I apparently added...

thafryer · 2023-10-16T18:59:14Z

🚀 PR was released in v0.0.27 🚀

✅ Extract utility functions for tests into separate file.

eeb25b7

This extracts the utility functions for comparing archives into a separate file, testUtils.

jwir3 requested review from tmeasday and skitterm October 10, 2023 16:08

paulelliott reviewed Oct 10, 2023

View reviewed changes

src/write-archive/index.test.ts Outdated Show resolved Hide resolved

paulelliott reviewed Oct 10, 2023

View reviewed changes

src/write-archive/index.test.ts Outdated Show resolved Hide resolved

skitterm reviewed Oct 10, 2023

View reviewed changes

skitterm requested a review from tevanoff October 10, 2023 21:52

tmeasday reviewed Oct 11, 2023

View reviewed changes

src/write-archive/index.ts Show resolved Hide resolved

src/write-archive/index.test.ts Outdated Show resolved Hide resolved

jwir3 force-pushed the jwir3/cap-1203-enametoolong-error-during-archiving branch from 9cb64d2 to f385d57 Compare October 11, 2023 14:54

jwir3 force-pushed the jwir3/cap-1203-enametoolong-error-during-archiving branch from f385d57 to 6561537 Compare October 11, 2023 14:56

tevanoff reviewed Oct 11, 2023

View reviewed changes

tevanoff reviewed Oct 16, 2023

View reviewed changes

💄 Fix linting errors that were occurring because of TS.

5750afe

jwir3 force-pushed the jwir3/cap-1203-enametoolong-error-during-archiving branch from 6561537 to 5750afe Compare October 16, 2023 18:30

skitterm approved these changes Oct 16, 2023

View reviewed changes

jwir3 merged commit 0aa2195 into main Oct 16, 2023
2 checks passed

thafryer added the released label Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncate and map filenames that are too long in test archiver #24

Truncate and map filenames that are too long in test archiver #24

jwir3 commented Oct 10, 2023 •

edited by thafryer

linear bot commented Oct 10, 2023

skitterm left a comment

tmeasday left a comment

jwir3 commented Oct 11, 2023

tevanoff Oct 11, 2023

jwir3 Oct 11, 2023

tevanoff Oct 11, 2023 •

edited

jwir3 Oct 11, 2023

tevanoff Oct 16, 2023

jwir3 Oct 16, 2023

tmeasday commented Oct 11, 2023

jwir3 commented Oct 13, 2023 •

edited

tevanoff Oct 16, 2023

jwir3 Oct 16, 2023

thafryer commented Oct 16, 2023

Truncate and map filenames that are too long in test archiver #24

Truncate and map filenames that are too long in test archiver #24

Conversation

jwir3 commented Oct 10, 2023 • edited by thafryer

What Changed

How to test

Change Type

linear bot commented Oct 10, 2023

skitterm left a comment

Choose a reason for hiding this comment

tmeasday left a comment

Choose a reason for hiding this comment

jwir3 commented Oct 11, 2023

tevanoff Oct 11, 2023

Choose a reason for hiding this comment

jwir3 Oct 11, 2023

Choose a reason for hiding this comment

tevanoff Oct 11, 2023 • edited

Choose a reason for hiding this comment

jwir3 Oct 11, 2023

Choose a reason for hiding this comment

tevanoff Oct 16, 2023

Choose a reason for hiding this comment

jwir3 Oct 16, 2023

Choose a reason for hiding this comment

tmeasday commented Oct 11, 2023

jwir3 commented Oct 13, 2023 • edited

tevanoff Oct 16, 2023

Choose a reason for hiding this comment

jwir3 Oct 16, 2023

Choose a reason for hiding this comment

thafryer commented Oct 16, 2023

jwir3 commented Oct 10, 2023 •

edited by thafryer

tevanoff Oct 11, 2023 •

edited

jwir3 commented Oct 13, 2023 •

edited