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

Prevent extensions from depending on extensions #20412

Merged
merged 3 commits into from Jan 18, 2019

Conversation

jridgewell
Copy link
Contributor

Maybe would have prevented #20392

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM with one comment.

build-system/tasks/dep-check.js Show resolved Hide resolved
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

One more.

build-system/dep-check-config.js Show resolved Hide resolved
@cramforce
Copy link
Member

Wait, we didn't have this? Could it be that it was deleted at some point?

@jridgewell
Copy link
Contributor Author

Wait, we didn't have this? Could it be that it was deleted at some point?

I don't think we ever had it. Each extension would have to whitelist itself:

extensions/amp-img/**/*.js->extensions/amp-img/**/*.js
extensions/amp-ad/**/*.js->extensions/amp-ad/**/*.js

With this new code, that's allowed by default. Only the exceptional cases need to be added to the whitelist.

@jridgewell jridgewell merged commit 77111ba into ampproject:master Jan 18, 2019
@jridgewell jridgewell deleted the cross-extension-deps branch January 18, 2019 21:59
@jridgewell jridgewell added this to To do in Make Malte, Dima, Justin Happy via automation Feb 23, 2019
@jridgewell jridgewell moved this from To do to Done in Make Malte, Dima, Justin Happy Feb 23, 2019
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Prevent extensions from depending on extensions

* Fix

* Fix whitelist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants