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 ignore minimatch to include folders starting with dot #8074

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Conversation

amiramw
Copy link
Member

@amiramw amiramw commented Jun 22, 2020

for example during npm install there is a temp folder called .staging and we don't want to listen to those files.

Signed-off-by: Amiram Wingarten amiram.wingarten@sap.com

What it does

Add minimatch dot option to make sure also folders starting with dot are excluded from watched. See also https://github.com/isaacs/minimatch#dot
The new behavior complies also with VS Code watch ignore behavior.

How to test

  1. Change suffix of helloworld-sample-0.0.1.zip to .vsix and deploy it. It prints to output on each package.json changed.
  2. Open some node project with node_modules folder.
  3. Under node_modules/.bin folder create package.json file.

Before this PR there was an output message to output channel "Package.json Events" which is wrong. After this PR there is no such ourput.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added the filesystem issues related to the filesystem label Jun 23, 2020
@akosyakov
Copy link
Member

@amiramw Could you check please why build is failing?

@amiramw
Copy link
Member Author

amiramw commented Jun 23, 2020

@akosyakov can you help me understand why is it failing? It's not clear to me from the logs. is it stable? I see that it only failed on linux. maybe should try to retrigger?

for example during npm install there is a temp folder called .staging and we don't want to listen to those files

Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
@amiramw
Copy link
Member Author

amiramw commented Jun 23, 2020

@akosyakov after rebase it seems to pass

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@amiramw do you perhaps have the code of the extension hosted anywhere (ex: github repository), I wanted to see what it does or expects. I'm not sure what the pull-request is trying to fix, for example when a package.json file under .bin is opened it is being watched:

root INFO [nsfw-watcher: 15227] Started watching: /home/evinfug/workspaces/theia/node_modules/.bin/package.json
root INFO [nsfw-watcher: 15227] Stopped watching: /home/evinfug/workspaces/theia/node_modules/.bin/package.json

What problem is the pull-request solving, it was not clear to me by the description.

@amiramw
Copy link
Member Author

amiramw commented Jun 23, 2020

@vince-fugnitto the git repo for this test extension is https://github.com/amiramw/vscode-file-watch-package-json

It basically listen to package.json file changes and print those events to output:

	const watcher = vscode.workspace.createFileSystemWatcher("**/package.json");

	const outputChannel = vscode.window.createOutputChannel('Package.json Events');
	watcher.onDidCreate(uri => outputChannel.appendLine('Created ' + uri.toString()));
	watcher.onDidChange(uri => outputChannel.appendLine('Changed ' + uri.toString()));

nsfw listens to every file change however fireDidFilesChanged is triggered only to events which are not ignored. The decision about ignore is in:

protected isIgnored(watcherId: number, path: string): boolean {
const options = this.watcherOptions.get(watcherId);
return !!options && options.ignored.length > 0 && options.ignored.some(m => m.match(path));
}

The ignored glob pattern is in settings.json and the default value is:

    "files.watcherExclude": {
        "**/.git/objects/**": true,
        "**/.git/subtree-cache/**": true,
        "**/node_modules/**": true
    }

My fix is to configure the ignore glob pattern minimatch so that it will also identify folders starting with dot in the path. Otherwise events are fired also for paths inside node_modules which is wrong and incompatible with vs code.

The vscode extension I provided test that.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@amiramw thank you for taking the time to describe the issue further and the use of dot.
Logically the change makes sense and I verified that the logs are not outputted to the output channel with your updates.

@vince-fugnitto vince-fugnitto added the file-watchers issues related to filesystem watchers - nsfw label Jun 23, 2020
@amiramw amiramw merged commit 3eb0678 into master Jun 24, 2020
@amiramw amiramw deleted the dot branch June 24, 2020 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-watchers issues related to filesystem watchers - nsfw filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants