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

feat: Added --ignore-files option and build now ignores artifacts folder #753

Merged
merged 42 commits into from
Feb 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
19504b1
feat: add artifactsDir to ignoreFileList
eight04 Jan 15, 2017
cd70980
feat: add --ignore-files option to build command
eight04 Jan 15, 2017
b48244d
test: --ignore-files option, ignore artifacts dir
eight04 Jan 15, 2017
c497465
fix: resolve to absolute path
eight04 Jan 15, 2017
07cc1a1
fix: make ignore-files option global
eight04 Jan 16, 2017
e9e583a
refactor: split FileFilter out from build.js to util/file-filter.js
eight04 Jan 16, 2017
dd3188b
fix: make FileFilter accept sourceDir, ignoreFiles, artifactsDir
eight04 Jan 16, 2017
619342d
fix: let build.js build fileFilter with more options
eight04 Jan 16, 2017
0cb2a43
fix: let lint.js build FileFilter with more options
eight04 Jan 16, 2017
cae516e
fix: make run.js be able to pass shouldWatchFile into watcher
eight04 Jan 16, 2017
30254c5
fix: make reloadStrategy accept ignoreFiles
eight04 Jan 17, 2017
f51841b
fix: better description for --ignore-files
eight04 Jan 17, 2017
de6ecfd
test: test new options: artifactsDir, sourceDir, ignoreFiles
eight04 Jan 17, 2017
6e4d8ae
fix: normalize relative path and correctly ignore a directory
eight04 Jan 17, 2017
173333b
refactor: split filt-filter test out from test.build.js to unit/test-…
eight04 Jan 17, 2017
fc02fc2
test: normalizeResolve
eight04 Jan 17, 2017
452e382
test: rewrite ignore artifacts dir when zipping test
eight04 Jan 17, 2017
87f30bc
fix: minor fix in normalizeResolve
eight04 Jan 17, 2017
21e582a
Merge branch 'master' of https://github.com/mozilla/web-ext into dev-…
eight04 Jan 19, 2017
4b8fc84
Revert "test: --ignore-files option, ignore artifacts dir"
eight04 Jan 20, 2017
ea48a6f
fix: add createFileFilter for testing
eight04 Jan 20, 2017
7d7a83e
fix: replace FileFilter with createFileFilter for testing
eight04 Jan 20, 2017
bd331ce
test: ensure lint.js use createFileFilter correctly
eight04 Jan 20, 2017
cf64c51
test: defaultWatcherCreator should use shouldWatchFile in watcher
eight04 Jan 20, 2017
1d86e35
fix: make run.js use fileFilterCreator
eight04 Jan 20, 2017
c51c936
test: ensure run.js use ignoreFile and createFileFilter correctly
eight04 Jan 20, 2017
e03c90f
fix: make build.js use createFileFilter
eight04 Jan 20, 2017
6512fe4
test: add configure a build test
eight04 Jan 20, 2017
f30e931
test: add shouldScanFile test
eight04 Jan 20, 2017
71e3da4
Merge branch 'master' of https://github.com/mozilla/web-ext into dev-…
eight04 Jan 20, 2017
4113539
fix: shouldWatchFile should be built in defaultWatcherCreator in run.js
eight04 Jan 23, 2017
000b7f2
fix: minor fixes to comment and type creation
eight04 Jan 23, 2017
72f0808
fix: drop dir/ syntax in FileFilter
eight04 Jan 23, 2017
e95b538
fix: minor fixes to sentences, typo, log message
eight04 Jan 25, 2017
0d35950
fix: minor fixes to file-filter
eight04 Jan 25, 2017
6f61733
test: drop calledWith
eight04 Jan 25, 2017
1ad7c5b
fix: type check
eight04 Jan 25, 2017
1795c0e
fix: type check
eight04 Jan 25, 2017
214e09e
fix: minor fixes to test/command messages
eight04 Feb 1, 2017
09bee46
fix: use path.join instead of path.sep
eight04 Feb 1, 2017
70db989
fix: use requiresArg, demand for ignore-files
eight04 Feb 1, 2017
6e45154
fix: change FileFilter param filesToIgnore -> baseIgnoredPatterns
eight04 Feb 1, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 17 additions & 43 deletions src/cmd/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import path from 'path';
import {createWriteStream} from 'fs';

import minimatch from 'minimatch';
import {fs} from 'mz';
import parseJSON from 'parse-json';
import eventToPromise from 'event-to-promise';
Expand All @@ -13,9 +12,14 @@ import getValidatedManifest, {getManifestId} from '../util/manifest';
import {prepareArtifactsDir} from '../util/artifacts';
import {createLogger} from '../util/logger';
import {UsageError} from '../errors';
import {
createFileFilter as defaultFileFilterCreator,
FileFilter,
} from '../util/file-filter';
// Import flow types.
import type {OnSourceChangeFn} from '../watcher';
import type {ExtensionManifest} from '../util/manifest';
import type {FileFilterCreatorFn} from '../util/file-filter';

const log = createLogger(__filename);

Expand Down Expand Up @@ -139,25 +143,34 @@ export type BuildCmdParams = {|
sourceDir: string,
artifactsDir: string,
asNeeded?: boolean,
ignoreFiles?: Array<string>,
|};

export type BuildCmdOptions = {|
manifestData?: ExtensionManifest,
fileFilter?: FileFilter,
onSourceChange?: OnSourceChangeFn,
packageCreator?: PackageCreatorFn,
showReadyMessage?: boolean
showReadyMessage?: boolean,
createFileFilter?: FileFilterCreatorFn,
|};

export default async function build(
{sourceDir, artifactsDir, asNeeded = false}: BuildCmdParams,
{sourceDir, artifactsDir, asNeeded = false, ignoreFiles = []}: BuildCmdParams,
{
manifestData, fileFilter = new FileFilter(),
manifestData,
createFileFilter = defaultFileFilterCreator,
fileFilter = createFileFilter({
sourceDir,
artifactsDir,
ignoreFiles,
}),
onSourceChange = defaultSourceWatcher,
packageCreator = defaultPackageCreator,
showReadyMessage = true,
}: BuildCmdOptions = {}
): Promise<ExtensionBuildResult> {

const rebuildAsNeeded = asNeeded; // alias for `build --as-needed`
log.info(`Building web extension from ${sourceDir}`);

Expand Down Expand Up @@ -185,42 +198,3 @@ export default async function build(

return result;
}


// FileFilter types and implementation.

export type FileFilterOptions = {|
filesToIgnore?: Array<string>,
|};

/*
* Allows or ignores files when creating a ZIP archive.
*/
export class FileFilter {
filesToIgnore: Array<string>;

constructor({filesToIgnore}: FileFilterOptions = {}) {
this.filesToIgnore = filesToIgnore || [
'**/*.xpi',
'**/*.zip',
'**/.*', // any hidden file
'**/node_modules',
];
}

/*
* Returns true if the file is wanted for the ZIP archive.
*
* This is called by zipdir as wantFile(path, stat) for each
* file in the folder that is being archived.
*/
wantFile(path: string): boolean {
for (const test of this.filesToIgnore) {
if (minimatch(path, test)) {
log.debug(`FileFilter: ignoring file ${path}`);
return false;
}
}
return true;
}
}
17 changes: 12 additions & 5 deletions src/cmd/lint.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@
import {createInstance as defaultLinterCreator} from 'addons-linter';

import {createLogger} from '../util/logger';
import {FileFilter} from './build';

import {
createFileFilter as defaultFileFilterCreator,
} from '../util/file-filter';
// import flow types
import type {FileFilterCreatorFn} from '../util/file-filter';

const log = createLogger(__filename);

Expand Down Expand Up @@ -46,23 +49,27 @@ export type LintCmdParams = {|
metadata?: boolean,
pretty?: boolean,
warningsAsErrors?: boolean,
ignoreFiles?: Array<string>,
artifactsDir?: string,
|};

export type LintCmdOptions = {|
createLinter?: LinterCreatorFn,
fileFilter?: FileFilter,
createFileFilter?: FileFilterCreatorFn,
|};

export default function lint(
{
verbose, sourceDir, selfHosted, boring, output,
metadata, pretty, warningsAsErrors,
metadata, pretty, warningsAsErrors, ignoreFiles, artifactsDir,
}: LintCmdParams,
{
createLinter = defaultLinterCreator,
fileFilter = new FileFilter(),
createFileFilter = defaultFileFilterCreator,
}: LintCmdOptions = {}
): Promise<void> {
const fileFilter = createFileFilter({sourceDir, ignoreFiles, artifactsDir});

log.debug(`Running addons-linter on ${sourceDir}`);
const linter = createLinter({
config: {
Expand Down
25 changes: 20 additions & 5 deletions src/cmd/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import {
import {createLogger} from '../util/logger';
import getValidatedManifest, {getManifestId} from '../util/manifest';
import defaultSourceWatcher from '../watcher';
import {
createFileFilter as defaultFileFilterCreator,
} from '../util/file-filter';
// Import objects that are only used as Flow types.
import type {FirefoxPreferences} from '../firefox/preferences';
import type {OnSourceChangeFn} from '../watcher';
Expand All @@ -26,6 +29,7 @@ import type {
FirefoxRDPResponseAddon,
} from '../firefox/remote';
import type {ExtensionManifest} from '../util/manifest';
import type {FileFilterCreatorFn} from '../util/file-filter';

const log = createLogger(__filename);

Expand All @@ -38,18 +42,23 @@ export type WatcherCreatorParams = {|
artifactsDir: string,
onSourceChange?: OnSourceChangeFn,
desktopNotifications?: typeof defaultDesktopNotifications,
ignoreFiles?: Array<string>,
createFileFilter?: FileFilterCreatorFn,
|};


export type WatcherCreatorFn = (params: WatcherCreatorParams) => Watchpack;

export function defaultWatcherCreator(
{
addonId, client, sourceDir, artifactsDir,
addonId, client, sourceDir, artifactsDir, ignoreFiles,
onSourceChange = defaultSourceWatcher,
desktopNotifications = defaultDesktopNotifications,
createFileFilter = defaultFileFilterCreator,
}: WatcherCreatorParams
): Watchpack {
const fileFilter = createFileFilter(
{sourceDir, artifactsDir, ignoreFiles}
);
return onSourceChange({
sourceDir,
artifactsDir,
Expand All @@ -67,6 +76,7 @@ export function defaultWatcherCreator(
throw error;
});
},
shouldWatchFile: (file) => fileFilter.wantFile(file),
});
}

Expand All @@ -80,22 +90,25 @@ export type ReloadStrategyParams = {|
profile: FirefoxProfile,
sourceDir: string,
artifactsDir: string,
ignoreFiles?: Array<string>,
|};

export type ReloadStrategyOptions = {|
createWatcher?: WatcherCreatorFn,
createFileFilter?: FileFilterCreatorFn,
|};

export function defaultReloadStrategy(
{
addonId, firefoxProcess, client, profile, sourceDir, artifactsDir,
addonId, firefoxProcess, client, profile,
sourceDir, artifactsDir, ignoreFiles,
}: ReloadStrategyParams,
{
createWatcher = defaultWatcherCreator,
}: ReloadStrategyOptions = {}
): void {
const watcher: Watchpack = (
createWatcher({addonId, client, sourceDir, artifactsDir})
createWatcher({addonId, client, sourceDir, artifactsDir, ignoreFiles})
);

firefoxProcess.on('close', () => {
Expand Down Expand Up @@ -165,6 +178,7 @@ export type CmdRunParams = {|
browserConsole: boolean,
customPrefs?: FirefoxPreferences,
startUrl?: string | Array<string>,
ignoreFiles?: Array<string>,
|};

export type CmdRunOptions = {|
Expand All @@ -177,7 +191,7 @@ export default async function run(
{
sourceDir, artifactsDir, firefox, firefoxProfile,
preInstall = false, noReload = false,
browserConsole = false, customPrefs, startUrl,
browserConsole = false, customPrefs, startUrl, ignoreFiles,
}: CmdRunParams,
{
firefoxApp = defaultFirefoxApp,
Expand Down Expand Up @@ -264,6 +278,7 @@ export default async function run(
sourceDir,
artifactsDir,
addonId,
ignoreFiles,
});
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,15 @@ Example: $0 --help run.
describe: 'Show verbose output',
type: 'boolean',
},
'ignore-files': {
Copy link
Member

Choose a reason for hiding this comment

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

this options should require an argument (by adding the requiresArg: true to the yargs properties for the option).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that setting requiresArg would force yargs to validate ignoreFiles is not a empty list. Since the ignoreFiles defaults to an empty list, this make yargs always argue Missing argument value: ignore-files.

Copy link
Member

Choose a reason for hiding this comment

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

This comment is supposed to be part of the last review comments but it has not been submitted by github, and so I added it here as a separate comment.

@eight04 yargs can be very annoying sometimes :-)

Anyway, it looks like that we can use a combination of yargs option config properties to make it work as expected:

    'ignore-files': {
      alias: 'i',
      describe: '...',
      demand: false,
      requiresArg: true,
      type: 'array',
    },

Basically it seems that we have to:

  • omit the empty array currently set as its default value
  • add a demand: false config to allow the --ignore-files option to be undefined (which is going to mean "that the option has not been specified")
  • add the requiresArg: true which force "--ignore-files" to have a value when specified

I briefly tried the above config locally and it seems to work as expected,
let me know if it doesn't for you.

alias: 'i',
describe: 'A list of glob patterns to define which files should be ' +
'ignored. (Example: --ignore-files=path/to/first.js ' +
'path/to/second.js "**/*.log")',
demand: false,
requiresArg: true,
type: 'array',
},
});

program
Expand Down
119 changes: 119 additions & 0 deletions src/util/file-filter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/* @flow */
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call on moving this to its own file. You also need to move the tests out of test.build.js. The new ignoreFiles needs test coverage too.

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 found some bugs when writing tests for FileFilter.

    it('ignore artifactsDir and its content', () => {
      const filter = new FileFilter({
        artifactsDir: 'artifacts',
      });
      assert.equal(filter.wantFile('artifacts'), false);
      assert.equal(filter.wantFile('artifacts/some.js'), false); // assertion error
    });

It might be fine for build.js, since the zipDir will skip the content if the parent dir is skipped.

But it cause problem for watcher.js. Make the content inside artifactsDir becomes wanted files. This bug applies to .git and node_modules folders as well.

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'm not sure how should we fix it. Just explicitly ignore all content inside the folder?

    filesToIgnore = [
      '**/*.xpi',
      '**/*.zip',
      '**/.*', // any hidden file
+     '**/.*/**/*', // any content in the hidden folder
      '**/node_modules',
+     '**/node_modules/**/*',
    ],

Copy link
Contributor

Choose a reason for hiding this comment

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

These issues are both resolved in the patch now.

import path from 'path';

import minimatch from 'minimatch';

import {createLogger} from './logger';

const log = createLogger(__filename);

// Use this function to mimic path.resolve without resolving to absolute path.
export const normalizeResolve = (file: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand why we need this function. Is it just for convenience so that someone can use an ignore path of src/package/../data/*.json instead of src/data/*.json? And the trailing slash protection is also for convenience? I feel like we shouldn't impose too many constraints on the ignoreFiles logic since that's up to the developer to use for customization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/eight04/web-ext/blob/71e3da42208cbca3fc98296c2e16c2a18f82d6fb/src/util/file-filter.js#L65
I want to make sure the result returned from FileFilter.resolve is consistent. Currently fileFilter.resolve has two behaviors.

  1. If sourceDir is set, all relative paths will be converted into absolute path by path.resolve(sourceDir, file), which can represent as path.join -> path.normalize -> trim trailing slash.
  2. Otherwise, just do path.normalize -> trim trailing slash to the path.

This make sourceDir optional and fileFilter can be run in "relative mode", which is the original behavior in test.build.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that makes sense. Consistency is good.

// normalize
file = path.normalize(file);

// trim trailing slash
if (path.parse(file).base && file.endsWith(path.sep)) {
return file.slice(0, -1);
}
return file;
};

// FileFilter types and implementation.

export type FileFilterOptions = {|
baseIgnoredPatterns?: Array<string>,
ignoreFiles?: Array<string>,
sourceDir?: string,
artifactsDir?: string,
|};

/*
* Allows or ignores files.
*/
export class FileFilter {
filesToIgnore: Array<string>;
sourceDir: ?string;

constructor({
baseIgnoredPatterns = [
'**/*.xpi',
'**/*.zip',
'**/.*', // any hidden file and folder
'**/.*/**/*', // and the content inside hidden folder
'**/node_modules',
'**/node_modules/**/*',
],
ignoreFiles = [],
sourceDir,
artifactsDir,
}: FileFilterOptions = {}) {

this.filesToIgnore = [];
this.sourceDir = sourceDir;

this.addToIgnoreList(baseIgnoredPatterns);
if (ignoreFiles) {
this.addToIgnoreList(ignoreFiles);
}
if (artifactsDir) {
this.addToIgnoreList([
artifactsDir,
path.join(artifactsDir, '**', '*'),
]);
}
}

/**
* Resolve relative path to absolute path if sourceDir is setted.
*/
resolve(file: string): string {
if (this.sourceDir) {
Copy link
Contributor

@kumar303 kumar303 Jan 24, 2017

Choose a reason for hiding this comment

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

I am a little bit worried about pre-pending sourceDir because it might be surprising to developers. How about adding a debug statement:

log.debug(`Adding sourceDir ${this.sourceDir} to the beginning of file ${file}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prepending only happens if sourceDir is set and file is relative. Should we check whether the file is relative to log the info?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, the debug statement now shows if the path is relative or not.

const resolvedPath = path.resolve(this.sourceDir, file);
log.debug(
`Resolved path ${file} with sourceDir ${this.sourceDir || ''} ` +
`to ${resolvedPath}`
);
return resolvedPath;
}
return normalizeResolve(file);
}

/**
* Insert more files into filesToIgnore array.
*/
addToIgnoreList(files: Array<string>) {
for (const file of files) {
this.filesToIgnore.push(this.resolve(file));
}
}

/*
* Returns true if the file is wanted.
*
* If path does not start with a slash, it will be treated as a path
* relative to sourceDir when matching it against all configured
* ignore-patterns.
*
* This is called by zipdir as wantFile(path, stat) for each
* file in the folder that is being archived.
*/
wantFile(path: string): boolean {
path = this.resolve(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking more about this and I don't think it's right to always assume sourceDir is the root. Can't we use minimatch(..., {matchBase: true}) to solve the problem?

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'm not sure what is the problem described here. matchBase: true changes the behavior of match pattern that doesn't contain slashes, make *.js behaves like **/*.js.

We normalize the path here is to make sure it can match normalized patterns, so fileFilter.wantFile("path/to/file") works correctly on Windows. Normally the path is already normalized since zip-dir, watchpack, and addons-linter always pass back full, normalized path, so this change only affect our test.file-filter.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, whoops, matchBase is not right then. I was thinking more about how sourceDir gets prepended to all patterns.

for (const test of this.filesToIgnore) {
if (minimatch(path, test)) {
log.debug(`FileFilter: ignoring file ${path} (it matched ${test})`);
return false;
}
}
return true;
}
}

// a helper function to make mocking easier

export const createFileFilter = (
(params: FileFilterOptions): FileFilter => new FileFilter(params)
);

export type FileFilterCreatorFn = typeof createFileFilter;
2 changes: 1 addition & 1 deletion src/watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Watchpack from 'watchpack';
import debounce from 'debounce';

import {createLogger} from './util/logger';
import {FileFilter} from './cmd/build';
import {FileFilter} from './util/file-filter';


const log = createLogger(__filename);
Expand Down
Loading