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

Expose watcher ignore and watcher backend options #9547

Merged
merged 13 commits into from
Mar 14, 2024

Conversation

Nikola-3
Copy link
Contributor

@Nikola-3 Nikola-3 commented Feb 27, 2024

↪️ Pull Request

Adds CLI options to set watcher backend and paths/globs for watcher to ignore. Note ignoring only means relevant events are not passed from OS to js runtime, the events will still be watched natively as there is no way to completely ignore them.

💻 Examples

Now it is possible to run:

  • parcel build ./index.html --watcher-backend fsevents
  • parcel build ./index.html --watcher-ignore ./.yarn ./**/.git

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

return {
shouldDisableCache: command.cache === false,
cacheDir: command.cacheDir,
watchDir: command.watchDir,
baseWatcherOptions: {
backend: command.watcherBackend,
ignore: command.watcherIgnoreDirs ?? ['.git', '.hg'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like .git and .hg should always be ignored regardless. Maybe just leave them hardcoded in getWatcherOptions?

@@ -189,6 +189,9 @@ export default async function resolveOptions(
shouldTrace: initialOptions.shouldTrace ?? false,
cacheDir,
watchDir,
baseWatcherOptions: initialOptions.baseWatcherOptions ?? {
Copy link
Contributor

Choose a reason for hiding this comment

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

/nit or /bikeshed - should this just be called watcherOptions? I know it's not all the watcher options, but from a consumer of the API point of view it's the options you can specify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't go for watcherOptions is because I feel like it's somewhat misleading/suggestive precisely since there are other options (e.g. watchDir) that configure the watcher. Reading base... implies there may be others. Happy to change if you still feel watcherOptions is good enough.

@@ -285,6 +285,11 @@ export type DetailedReportOptions = {|
assetsPerBundle?: number,
|};

export type WatcherOptions = {|
...BaseWatcherOptions,
ignore: BaseWatcherOptions['ignore'],
Copy link
Contributor

Choose a reason for hiding this comment

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

huh, neat, I didn't realise you could reference part of a type like that with Flow!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping this would make it non-nullable, but apparently, the preferred way is to wrap the type with $NonMaybeType<MaybeType>. It's not as neat though, the vscode server doesn't display the particular type.

@@ -75,6 +75,11 @@ const commonOptions = {
'--cache-dir <path>': 'set the cache directory. defaults to ".parcel-cache"',
'--watch-dir <path>':
'set the root watch directory. defaults to nearest lockfile or source control dir.',
'--watcher-ignore-dirs [path]': `list of directories watcher should not be tracking for changes. defaults to ['.git', '.hg']`,
Copy link
Member

Choose a reason for hiding this comment

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

I think they don't need to be directories, the watcher also accepts globs. so maybe just --watcher-ignore? or maybe --watch-ignore for consistency with --watch-dir?

@@ -18,6 +18,7 @@ cache.ensure();
export const DEFAULT_OPTIONS: ParcelOptions = {
cacheDir: path.join(__dirname, '.parcel-cache'),
watchDir: __dirname,
baseWatcherOptions: {},
Copy link
Member

Choose a reason for hiding this comment

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

Should we just call it watcherOptions? Why the base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make it clear that these are not the only options that exist that can be set for the watcher, e.g. the watchDir option above it

'--watcher-backend': new commander.Option(
'--watcher-backend <name>',
'set watcher backend',
).choices(['fs-events', 'watchman', 'inotify', 'windows', 'brute-force']),
Copy link
Member

Choose a reason for hiding this comment

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

wonder if the allowed options should depend on the platform parcel is running on. e.g. inotify is only available on linux, fs-events is only available on Mac, etc.

Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

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

@Nikola-3 We'll need to add the watcher backend to the cache key as they have different snapshot formats.

@Nikola-3 Nikola-3 force-pushed the add-cli-options-for-watcher-backend-and-ignore-dirs branch from 16e5201 to 7e494e8 Compare March 11, 2024 03:38
@mattcompiles mattcompiles changed the title Expose watcher options Expose watcher ignore and watcher backend options Mar 13, 2024
@mattcompiles mattcompiles merged commit e9bf419 into v2 Mar 14, 2024
14 of 16 checks passed
@mattcompiles mattcompiles deleted the add-cli-options-for-watcher-backend-and-ignore-dirs branch March 14, 2024 03:09
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.

5 participants