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

Move analytics services into extension directory and make dep check stricter. #3147

Merged
merged 1 commit into from
May 7, 2016

Conversation

cramforce
Copy link
Member

@cramforce cramforce commented May 7, 2016

The dep check now has a whitelist that allows fine grained whitelisting.

@cramforce
Copy link
Member Author

CC @avimehta @erwinmombay

@cramforce cramforce force-pushed the stricter-dep-check branch 2 times, most recently from 2f4312a to b9f312e Compare May 7, 2016 05:06
@erwinmombay
Copy link
Member

@cramforce does this affect the build size? (can you run gulp size if so)

@@ -34,6 +34,25 @@ exports.rules = [
{
filesMatching: 'extensions/**/*.js',
mustNotDependOn: 'src/service/**/*.js',
whitelist: 'extensions/**/amp-analytics.js',
},
// Files under src must under depend on extensions.
Copy link
Member

Choose a reason for hiding this comment

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

"must under depend", is that right?

@cramforce cramforce changed the title Move analytics services into extension directory. Move analytics services into extension directory and make dep check stricter. May 7, 2016
…stricter.

The dep check now has a whitelist that allows fine grained whitelisting.
@cramforce
Copy link
Member Author

Should not affect build size, but can run.

Slightly updated PR to do whitelisting in a way that is more directed.

@cramforce
Copy link
Member Author

There are some size changes, but I don't think they come from this. It should only move files around unless you see anything. The dependency graph is unchanged.

@@ -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('->');
Copy link
Member

Choose a reason for hiding this comment

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

nice feature 👍

@erwinmombay
Copy link
Member

@cramforce LGTM. awesome feature addition.

@cramforce cramforce merged commit 42ae6c7 into ampproject:master May 7, 2016
@cramforce cramforce deleted the stricter-dep-check branch May 7, 2016 15:03
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