Move mutator implementations out to a standalone service#25999
Move mutator implementations out to a standalone service#25999powerivq merged 1 commit intoampproject:masterfrom
Conversation
lannka
left a comment
There was a problem hiding this comment.
Please do some sanity checks on different envs (shadowdoc, fie, inabox etc).
2c4acf3 to
c469e10
Compare
|
Adding @jpettitt for approval on amp-access-poool, @calebcordry for amp-story-auto-ad |
f709771 to
4a39a45
Compare
|
Story ads lgtm. |
073080c to
58fb26c
Compare
58fb26c to
be0c5f1
Compare
|
@choumx For approval of src/*. |
| /** | ||
| * Flag that the height could have been changed. | ||
| */ | ||
| maybeHeightChanged() {} |
There was a problem hiding this comment.
I think this is a great change overall, but a side effect is that many private methods are now public.
Can we avoid this API pollution? E.g. move Resources/Mutator/Owners into their own subfolder src/service/resources/ and mark previously-private methods with @package.
There was a problem hiding this comment.
That sounds great. That is an existing problem where we deal with today by using presubmit check to disallow any external dependency. The drawback is that js minimizer does not transform their method names, causing increased js bundle size.
Since this PR is already large. Can we merge this and then further refactor into a package?
There was a problem hiding this comment.
SGTM
The drawback is that js minimizer does not transform their method names, causing increased js bundle size.
This is true for both @public and @package methods right?
| /** | ||
| * Flag that the height could have been changed. | ||
| */ | ||
| maybeHeightChanged() {} |
There was a problem hiding this comment.
SGTM
The drawback is that js minimizer does not transform their method names, causing increased js bundle size.
This is true for both @public and @package methods right?
|
@choumx Not sure how closure compiler handles it, but I can certainly test! |
Phase 2 of #25296