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

Fix extension_directories wildcards to always be recursive #5160

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Jan 7, 2025

WHY are these changes introduced?

To improve the handling of custom extension directory paths in app configuration by ensuring proper glob pattern support for watching changes in subfolders.

WHAT is this pull request doing?

Adds functionality to automatically convert single asterisk wildcards (*) to double asterisks (**) in extension directory paths. This ensures proper recursive watching of subdirectories when using chokidar for file watching during dev.

How to test your changes?

  1. Create an app with and put some extensions in a folder called custom_extensions
  2. Ensure your app.toml has extension_directories = ['custom_extensions/*']
  3. Run dev
  4. Verify that changes in extension files are properly detected and HMR works.

Measuring impact

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@isaacroldan isaacroldan marked this pull request as ready for review January 7, 2025 15:22
@isaacroldan isaacroldan requested a review from a team as a code owner January 7, 2025 15:22
Copy link
Contributor

github-actions bot commented Jan 7, 2025

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.23% (+0.01% 🔼)
8859/11776
🟡 Branches
70.47% (+0.02% 🔼)
4297/6098
🟡 Functions
75.1% (+0.02% 🔼)
2319/3088
🟡 Lines
75.77% (+0.02% 🔼)
8375/11053
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%

Test suite run success

2000 tests passing in 903 suites.

Report generated by 🧪jest coverage report action from b874496

// If a path ends with a single asterisk, modify it to end with a double asterisk.
// This is to support the glob pattern used by chokidar and watch for changes in subfolders.
function fixSingleWildcards(value: string[] | undefined) {
return value?.map((dir) => dir.replace(/\*$/, '**'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this regex also convert a double-asterisk to a triple-asterisk? I think you might want a captured non-asterisk, followed by an asterisk and then end-of-line, as your regex:

Suggested change
return value?.map((dir) => dir.replace(/\*$/, '**'))
return value?.map((dir) => dir.replace(/([^\*])\*$/, '$1**'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this will work for any kind of path type / or \
I'll add tests to it though :)

@isaacroldan isaacroldan force-pushed the 01-07-fix_extension_directories_wildcards_to_always_be_recursive branch from 9d66cb8 to b874496 Compare January 8, 2025 09:55
@isaacroldan isaacroldan requested a review from dmerand January 8, 2025 10:02
Copy link
Contributor

@dmerand dmerand left a comment

Choose a reason for hiding this comment

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

Tophat LGTM! Thanks for the changes.

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