Skip to content

Refactor EmbeddableService #19771

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

Merged
merged 13 commits into from
Dec 17, 2018
Merged

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Dec 10, 2018

Partial for #16322. Follow-up to #19344.

When installing a service in an FIE, remove the requirement that it must be installed in the parent as well. For example, no need to install amp-bind in the parent if only the child FIE uses it.

  • Refactor EmbeddableService interface to have a static function (so we don't need an preexisting service instance).
  • Store the service class on the extension holder object (used to invoke the EmbeddableService static function).

@ghost

This comment has been minimized.

@ghost

This comment has been minimized.

@dreamofabear
Copy link
Author

dreamofabear commented Dec 13, 2018

/to @alabiaga @dvoytenko

@dvoytenko
Copy link
Contributor

@choumx LGTM. A little scary :)

@dreamofabear dreamofabear removed the request for review from alabiaga December 17, 2018 17:06
@dreamofabear
Copy link
Author

There's a bug with this PR in compiled mode. Closure Compiler will DCE static functions that are not invoked directly: google/closure-compiler#2763

Need to add @nocollapse to installinEmbedWindow() overrides.

@dreamofabear dreamofabear merged commit a352b31 into ampproject:master Dec 17, 2018
@dreamofabear dreamofabear deleted the fie-refactor-apis branch December 17, 2018 22:57
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Dec 19, 2018
* Refactor EmbeddableService.

* Half-fix test-extensions.

* Important flipped bool, fix test-extensions.

* Fix test-service.

* Fix test-action.

* Make StandardActions more like other FIE services.

* Fix test-navigation, test-standard-actions.

* Revert timer service change.

* Add @nocollapse to installInEmbedWindow() usages.
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Refactor EmbeddableService.

* Half-fix test-extensions.

* Important flipped bool, fix test-extensions.

* Fix test-service.

* Fix test-action.

* Make StandardActions more like other FIE services.

* Fix test-navigation, test-standard-actions.

* Revert timer service change.

* Add @nocollapse to installInEmbedWindow() usages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants