diff --git a/build-system/dep-check-config.js b/build-system/dep-check-config.js index 23f613369c2c..f4cac47782b4 100644 --- a/build-system/dep-check-config.js +++ b/build-system/dep-check-config.js @@ -19,7 +19,8 @@ * - filesMatching - Is assumed to be all files if not provided. * - mustNotDependOn - If type is "forbidden" (default) then the files * matched must not match the glob(s) provided. - * - whitelist - Skip rule if file matches whitelist pattern(s). + * - whitelist - Skip rule if this particular dependency is found. + * Syntax: fileA->fileB where -> reads "depends on" * @typedef {{ * type: (string|undefined), * filesMatching: (string|!Array|undefined), @@ -30,10 +31,25 @@ var RuleConfigDef; exports.rules = [ - // Extensions must not import any services directly. + // Extensions must not import any central services directly. { filesMatching: 'extensions/**/*.js', mustNotDependOn: 'src/service/**/*.js', - whitelist: 'extensions/**/amp-analytics.js', + }, + // Files under src must not depend on extensions. + { + filesMatching: 'src/**/*.js', + mustNotDependOn: 'extensions/**/*.js', + }, + // Files under src must not depend on ads code. + { + filesMatching: 'src/**/*.js', + mustNotDependOn: 'ads/**/*.js', + whitelist: 'src/ad-cid.js->ads/_config.js', + }, + // Files under src must not depend on 3p code. + { + filesMatching: 'src/**/*.js', + mustNotDependOn: '3p/**/*.js', }, ]; diff --git a/build-system/tasks/dep-check.js b/build-system/tasks/dep-check.js index 59a1e744cc65..5ab8c2fe59e0 100644 --- a/build-system/tasks/dep-check.js +++ b/build-system/tasks/dep-check.js @@ -75,7 +75,7 @@ function Rule(config) { /** @private @const {!GlobsDef} */ this.mustNotDependOn_ = toArrayOrDefault(config.mustNotDependOn, []); - /** @private @const {!GlobsDef} */ + /** @private @const {!Array} */ this.whitelist_ = toArrayOrDefault(config.whitelist, []); } @@ -86,13 +86,6 @@ function Rule(config) { */ Rule.prototype.run = function(moduleName, deps) { var errors = []; - var inWhitelist = this.whitelist_.length ? - this.whitelist_.some(x => minimist(moduleName, x)) : false; - - // Bail out early if its in the current rules whitelist. - if (inWhitelist) { - return errors; - } // If forbidden rule and current module has no dependencies at all // then no need to match. @@ -125,6 +118,18 @@ Rule.prototype.matchBadDeps = function(moduleName, deps) { deps.forEach(dep => { this.mustNotDependOn_.forEach(badDepPattern => { if (minimatch(dep, badDepPattern)) { + var inWhitelist = this.whitelist_.some(entry => { + var pair = entry.split('->'); + var whitelistedModuleName = pair[0]; + var whitelistedDep = pair[1]; + if (moduleName != whitelistedModuleName) { + return false; + } + return dep == whitelistedDep; + }); + if (inWhitelist) { + return; + } mustNotDependErrors.push(`${moduleName} must not depend on ${dep}. ` + `Rule: ${JSON.stringify(this.config_)}.`); } diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 2519c22a9dd2..8c0f8c04dda0 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -87,14 +87,14 @@ var forbiddenTerms = { 'installActivityService': { message: privateServiceFactory, whitelist: [ - 'src/service/activity-impl.js', + 'extensions/amp-analytics/0.1/activity-impl.js', 'extensions/amp-analytics/0.1/amp-analytics.js' ] }, 'installCidService': { message: privateServiceFactory, whitelist: [ - 'src/service/cid-impl.js', + 'extensions/amp-analytics/0.1/cid-impl.js', 'extensions/amp-access/0.1/amp-access.js', 'extensions/amp-analytics/0.1/amp-analytics.js', ], @@ -103,7 +103,7 @@ var forbiddenTerms = { message: privateServiceFactory, whitelist: [ 'extensions/amp-analytics/0.1/amp-analytics.js', - 'src/service/storage-impl.js', + 'extensions/amp-analytics/0.1/storage-impl.js', ], }, 'installUrlReplacementsService': { @@ -160,7 +160,7 @@ var forbiddenTerms = { message: privateServiceFactory, whitelist: [ 'src/service/viewer-impl.js', - 'src/service/storage-impl.js', + 'extensions/amp-analytics/0.1/storage-impl.js', 'examples/viewer-integr-messaging.js', 'extensions/amp-access/0.1/login-dialog.js', ], @@ -180,7 +180,7 @@ var forbiddenTerms = { 'getBaseCid': { message: requiresReviewPrivacy, whitelist: [ - 'src/service/cid-impl.js', + 'extensions/amp-analytics/0.1/cid-impl.js', 'src/service/viewer-impl.js', ], }, @@ -188,13 +188,13 @@ var forbiddenTerms = { message: requiresReviewPrivacy, whitelist: [ 'src/cookies.js', - 'src/service/cid-impl.js', + 'extensions/amp-analytics/0.1/cid-impl.js', ], }, 'getCookie\\W': { message: requiresReviewPrivacy, whitelist: [ - 'src/service/cid-impl.js', + 'extensions/amp-analytics/0.1/cid-impl.js', 'src/cookies.js', 'src/experiments.js', 'tools/experiments/experiments.js', @@ -203,7 +203,7 @@ var forbiddenTerms = { 'setCookie\\W': { message: requiresReviewPrivacy, whitelist: [ - 'src/service/cid-impl.js', + 'extensions/amp-analytics/0.1/cid-impl.js', 'src/cookies.js', 'src/experiments.js', 'tools/experiments/experiments.js', @@ -244,8 +244,8 @@ var forbiddenTerms = { 'localStorage': { message: requiresReviewPrivacy, whitelist: [ - 'src/service/cid-impl.js', - 'src/service/storage-impl.js', + 'extensions/amp-analytics/0.1/cid-impl.js', + 'extensions/amp-analytics/0.1/storage-impl.js', ], }, 'sessionStorage': requiresReviewPrivacy, diff --git a/extensions/amp-access/0.1/test/test-amp-access.js b/extensions/amp-access/0.1/test/test-amp-access.js index c7f1bc48b186..e6a822249c31 100644 --- a/extensions/amp-access/0.1/test/test-amp-access.js +++ b/extensions/amp-access/0.1/test/test-amp-access.js @@ -18,7 +18,8 @@ import {AccessClientAdapter} from '../amp-access-client'; import {AccessOtherAdapter} from '../amp-access-other'; import {AccessService} from '../amp-access'; import {Observable} from '../../../../src/observable'; -import {installCidService} from '../../../../src/service/cid-impl'; +import {installCidService,} from + '../../../../extensions/amp-analytics/0.1/cid-impl'; import {markElementScheduledForTesting} from '../../../../src/custom-element'; import * as sinon from 'sinon'; diff --git a/src/service/activity-impl.js b/extensions/amp-analytics/0.1/activity-impl.js similarity index 97% rename from src/service/activity-impl.js rename to extensions/amp-analytics/0.1/activity-impl.js index c7b2e7665356..79897357966b 100644 --- a/src/service/activity-impl.js +++ b/extensions/amp-analytics/0.1/activity-impl.js @@ -19,10 +19,10 @@ * has performed on the page. */ -import {getService} from '../service'; -import {viewerFor} from '../viewer'; -import {viewportFor} from '../viewport'; -import {listen} from '../event-helper'; +import {getService} from '../../../src/service'; +import {viewerFor} from '../../../src/viewer'; +import {viewportFor} from '../../../src/viewport'; +import {listen} from '../../../src/event-helper'; /** diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index f2736c995ba2..9b74ecbfc1df 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -19,10 +19,10 @@ import {addListener, instrumentationServiceFor} from './instrumentation'; import {assertHttpsUrl, addParamsToUrl} from '../../../src/url'; import {dev, user} from '../../../src/log'; import {expandTemplate} from '../../../src/string'; -import {installCidService} from '../../../src/service/cid-impl'; -import {installStorageService} from '../../../src/service/storage-impl'; -import {installActivityService} from '../../../src/service/activity-impl'; -import {installVisibilityService} from '../../../src/service/visibility-impl'; +import {installCidService} from './cid-impl'; +import {installStorageService} from './storage-impl'; +import {installActivityService} from './activity-impl'; +import {installVisibilityService} from './visibility-impl'; import {isArray, isObject} from '../../../src/types'; import {sendRequest, sendRequestUsingIframe} from './transport'; import {urlReplacementsFor} from '../../../src/url-replacements'; diff --git a/src/service/cid-impl.js b/extensions/amp-analytics/0.1/cid-impl.js similarity index 96% rename from src/service/cid-impl.js rename to extensions/amp-analytics/0.1/cid-impl.js index c59cdad28df1..c6b6a1976d23 100644 --- a/src/service/cid-impl.js +++ b/extensions/amp-analytics/0.1/cid-impl.js @@ -22,15 +22,19 @@ * For details, see https://goo.gl/Mwaacs */ -import {getCookie, setCookie} from '../cookies'; -import {getService} from '../service'; -import {getSourceOrigin, isProxyOrigin, parseUrl} from '../url'; -import {timer} from '../timer'; -import {viewerFor} from '../viewer'; +import {getCookie, setCookie} from '../../../src/cookies'; +import {getService} from '../../../src/service'; +import { + getSourceOrigin, + isProxyOrigin, + parseUrl, +} from '../../../src/url'; +import {timer} from '../../../src/timer'; +import {viewerFor} from '../../../src/viewer'; import { sha384Base64, -} from '../../third_party/closure-library/sha384-generated'; -import {user} from '../log'; +} from '../../../third_party/closure-library/sha384-generated'; +import {user} from '../../../src/log'; const ONE_DAY_MILLIS = 24 * 3600 * 1000; diff --git a/extensions/amp-analytics/0.1/instrumentation.js b/extensions/amp-analytics/0.1/instrumentation.js index 76685f0fb8f4..b2a557228866 100644 --- a/extensions/amp-analytics/0.1/instrumentation.js +++ b/extensions/amp-analytics/0.1/instrumentation.js @@ -15,7 +15,7 @@ */ import {isExperimentOn} from '../../../src/experiments'; -import {isVisibilitySpecValid} from '../../../src/service/visibility-impl'; +import {isVisibilitySpecValid} from './visibility-impl'; import {Observable} from '../../../src/observable'; import {getService} from '../../../src/service'; import {timer} from '../../../src/timer'; diff --git a/src/service/storage-impl.js b/extensions/amp-analytics/0.1/storage-impl.js similarity index 96% rename from src/service/storage-impl.js rename to extensions/amp-analytics/0.1/storage-impl.js index 532334deca44..8a558f6bea96 100644 --- a/src/service/storage-impl.js +++ b/extensions/amp-analytics/0.1/storage-impl.js @@ -14,12 +14,12 @@ * limitations under the License. */ -import {getService} from '../service'; -import {getSourceOrigin} from '../url'; -import {dev} from '../log'; -import {recreateNonProtoObject} from '../json'; -import {timer} from '../timer'; -import {viewerFor} from '../viewer'; +import {getService} from '../../../src/service'; +import {getSourceOrigin} from '../../../src/url'; +import {dev} from '../../../src/log'; +import {recreateNonProtoObject} from '../../../src/json'; +import {timer} from '../../../src/timer'; +import {viewerFor} from '../../../src/viewer'; /** @const */ const TAG = 'Storage'; diff --git a/extensions/amp-analytics/0.1/test/test-amp-analytics.js b/extensions/amp-analytics/0.1/test/test-amp-analytics.js index 45ac3d51860a..a95b68d5bff6 100644 --- a/extensions/amp-analytics/0.1/test/test-amp-analytics.js +++ b/extensions/amp-analytics/0.1/test/test-amp-analytics.js @@ -23,7 +23,8 @@ import {adopt} from '../../../../src/runtime'; import {createIframePromise} from '../../../../testing/iframe'; import {getService, resetServiceForTesting} from '../../../../src/service'; import {markElementScheduledForTesting} from '../../../../src/custom-element'; -import {installCidService} from '../../../../src/service/cid-impl'; +import {installCidService,} from + '../../../../extensions/amp-analytics/0.1/cid-impl'; import {installViewerService} from '../../../../src/service/viewer-impl'; import {installViewportService} from '../../../../src/service/viewport-impl'; import { diff --git a/src/service/visibility-impl.js b/extensions/amp-analytics/0.1/visibility-impl.js similarity index 97% rename from src/service/visibility-impl.js rename to extensions/amp-analytics/0.1/visibility-impl.js index d527880d03c0..e6439d585ced 100644 --- a/src/service/visibility-impl.js +++ b/extensions/amp-analytics/0.1/visibility-impl.js @@ -14,12 +14,12 @@ * limitations under the License. */ -import {dev} from '../log'; -import {getService} from '../service'; -import {resourcesFor} from '../resources'; -import {timer} from '../timer'; -import {user} from '../log'; -import {viewportFor} from '../viewport'; +import {dev} from '../../../src/log'; +import {getService} from '../../../src/service'; +import {resourcesFor} from '../../../src/resources'; +import {timer} from '../../../src/timer'; +import {user} from '../../../src/log'; +import {viewportFor} from '../../../src/viewport'; /** @const {number} */ const LISTENER_INITIAL_RUN_DELAY_ = 20; diff --git a/test/functional/test-activity.js b/test/functional/test-activity.js index 32c4a0588316..66960891c4f9 100644 --- a/test/functional/test-activity.js +++ b/test/functional/test-activity.js @@ -14,7 +14,8 @@ * limitations under the License. */ -import {installActivityService} from '../../src/service/activity-impl'; +import {installActivityService,} from + '../../extensions/amp-analytics/0.1/activity-impl'; import {activityFor} from '../../src/activity'; import {installViewerService} from '../../src/service/viewer-impl'; import {viewerFor} from '../../src/viewer'; diff --git a/test/functional/test-ad-cid.js b/test/functional/test-ad-cid.js index 1eec6af19133..5c51be36b118 100644 --- a/test/functional/test-ad-cid.js +++ b/test/functional/test-ad-cid.js @@ -18,7 +18,7 @@ import {clientIdScope} from '../../ads/_config'; import {createAdPromise} from '../../testing/ad-iframe'; import {installAd} from '../../builtins/amp-ad'; import {installEmbed} from '../../builtins/amp-embed'; -import {installCidService} from '../../src/service/cid-impl'; +import {installCidService} from '../../extensions/amp-analytics/0.1/cid-impl'; import { installUserNotificationManager, } from '../../extensions/amp-user-notification/0.1/amp-user-notification'; diff --git a/test/functional/test-cid.js b/test/functional/test-cid.js index 5d7f47d9f0f6..3d9992ec646e 100644 --- a/test/functional/test-cid.js +++ b/test/functional/test-cid.js @@ -18,7 +18,7 @@ import {cidFor} from '../../src/cid'; import { installCidService, getProxySourceOrigin, -} from '../../src/service/cid-impl'; +} from '../../extensions/amp-analytics/0.1/cid-impl'; import {parseUrl} from '../../src/url'; import {timer} from '../../src/timer'; import {installViewerService} from '../../src/service/viewer-impl'; diff --git a/test/functional/test-storage.js b/test/functional/test-storage.js index 1797e3c000b2..5eb742349cb5 100644 --- a/test/functional/test-storage.js +++ b/test/functional/test-storage.js @@ -19,7 +19,7 @@ import { Store, LocalStorageBinding, ViewerStorageBinding, -} from '../../src/service/storage-impl'; +} from '../../extensions/amp-analytics/0.1/storage-impl'; import * as sinon from 'sinon'; diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index 909f6f9c9b83..e98c61f8640a 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -19,9 +19,10 @@ import {createIframePromise} from '../../testing/iframe'; import {user} from '../../src/log'; import {urlReplacementsFor} from '../../src/url-replacements'; import {markElementScheduledForTesting} from '../../src/custom-element'; -import {installCidService} from '../../src/service/cid-impl'; +import {installCidService} from '../../extensions/amp-analytics/0.1/cid-impl'; import {installViewerService} from '../../src/service/viewer-impl'; -import {installActivityService} from '../../src/service/activity-impl'; +import {installActivityService,} from + '../../extensions/amp-analytics/0.1/activity-impl'; import { installUrlReplacementsService, } from '../../src/service/url-replacements-impl'; diff --git a/test/functional/test-visibility.js b/test/functional/test-visibility.js index 9414a6ed0e0f..8f03e930347d 100644 --- a/test/functional/test-visibility.js +++ b/test/functional/test-visibility.js @@ -14,8 +14,12 @@ * limitations under the License. */ -import {isPositiveNumber_, isValidPercentage_, isVisibilitySpecValid, - installVisibilityService} from '../../src/service/visibility-impl'; +import { + isPositiveNumber_, + isValidPercentage_, + isVisibilitySpecValid, + installVisibilityService, +} from '../../extensions/amp-analytics/0.1/visibility-impl'; import {installResourcesService} from '../../src/service/resources-impl'; import {visibilityFor} from '../../src/visibility'; import * as sinon from 'sinon';