-
Notifications
You must be signed in to change notification settings - Fork 53
Refine export exclusion patterns #2349
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
Refine export exclusion patterns #2349
Conversation
📊 Performance Test ResultsComparing 05e5958 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix and sharing details in the PR description, Fredrik! I can confirm the changes address the issue which I cannot reproduce anymore. Site starts correctly without any missing files errors. 👍🏼
However, I noticed that the logic no longer filters out node_modules and cache directories during export.
Related Slack discussion with example site export: p1767625004464189/1767176661.251759-slack-C06DRMD6VPZ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update, Fredrik. The changes look good and work as expected on macOS. I did not observe any more regressions there. The files that should be excluded, get excluded.
On Windows the regex fails though:
Server started for '00000'
(node:6028) UnhandledPromiseRejectionWarning: SyntaxError: Invalid regular expression: /\(.git|node_modules|cache)(\|$)/g: Unmatched ')'
at new RegExp (<anonymous>)
at DefaultExporter.isPathExcludedByPattern (C:\Users\ivanottinger\Documents\GitHub\studio\dist\main\index.js:4537:27)
at C:\Users\ivanottinger\Documents\GitHub\studio\dist\main\index.js:4675:82
at Archiver.onGlobMatch (C:\Users\ivanottinger\Documents\GitHub\studio\node_modules\archiver\lib\core.js:659:21)
at ReaddirGlob.emit (node:events:519:28)
at C:\Users\ivanottinger\Documents\GitHub\studio\node_modules\readdir-glob\index.js:200:20
(node:6028) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 11)
Looks like we may need to escape the backslash in the regex pattern.
I left one additional (but minor) suggestion.
| ); | ||
| } ); | ||
|
|
||
| it( 'should exclude .git, node_modules, and cache directories', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding dedicated test cases.
| // a directory or not. | ||
| private isPathExcludedByPattern( pathToCheck: string ) { | ||
| const DIRECTORY_NAMES_TO_EXCLUDE = [ '.git', 'node_modules', 'cache' ]; | ||
| const offenderRegex = new RegExp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it wouldn't be nicer to rename offenderRegex to something like matchRegex and offenderPath to matchedPath (or something similar).
Offender sounds a bit negative to me. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but a negative connotation seems pretty appropriate to me in this context. We're naming something that matches a disallowed pattern and may be excluded from the export. "Match" also works, but I would opt for the added clarity of "offender", since there's no sensitive real-world association with that term.
|
Thanks for the alert reviews, @ivan-ottinger, and sorry about the sloppiness on my part 🤦♂️ I realized this morning that there was another problem with my regex: I also realized that the test was plain wrong. This comes down to my over-trusting of AI-implemented tests. I mentioned this elsewhere, too, but I know I'll be trying to flip my current workflow by writing the test first and then asking the AI to help with the implementation. It might not be as effective, but my own complacency with AI-generated tests is beginning to irk me. Anyway, I've added new tests that test the |
katinthehatsite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, I compared them to trunk and I can see the same set of files exported as before. I was also able to create a site with the exported file. I did not see anymore the issues mentioned by Ivan
Related issues
Proposed Changes
In #1264, we implemented some logic in
DefaultExporterto exclude.gitandnode_modulesfolders from export archives. The logic looked for those substrings anywhere in the path. This could be considered a risky approach, but since those terms are pretty esoteric, it likely didn't cause much trouble.In #1940, we added the term
cacheto this list, which is much more generic. When combined with the simplistic path-checking logic, this change would actually break exports containing files or directories with "cache" in the name that weren't meant to be excluded.STU-1189 documents one such example, where the Yoast SEO plugin would break during export (since certain required files were excluded). It's worth noting that sync pushes containing Yoast SEO plugins wouldn't have broken, since plugins aren't restored directly from the push archive on the back-end.
In any case, this PR addresses the issue by:
DefaultExporter::isPathExcludedByPattern.git,node_modules, orcacheis the full name of that directory.hellonode_modulesormy-cacheare not excluded.Testing Instructions
npm startPre-merge Checklist