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.

Sign up for GitHub

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

Jump to bottom

🚀 Separating BaseTemplate from template-impl. #30208

Merged
merged 23 commits into from
Sep 19, 2020
Merged

🚀 Separating BaseTemplate from template-impl. #30208

merged 23 commits into from
Sep 19, 2020

Conversation

xiexr151e
Copy link
Contributor

@xiexr151e xiexr151e commented Sep 12, 2020 •
edited
Loading

As @dvoytenko has pointed, BaseTemplate from service/template-impl.js could be refactored to reduce its size. This will be achieved by separating BaseTemplate.

@google-cla google-cla bot added the cla: yes label Sep 12, 2020
@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2020

This pull request introduces 3 alerts when merging dfe37a4 into 767775f - view on LGTM.com

new alerts:

  • 3 for Missing 'this' qualifier

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2020

This pull request introduces 1 alert when merging 9658906 into 767775f - view on LGTM.com

new alerts:

  • 1 for Missing 'this' qualifier

@dvoytenko
Copy link
Contributor

Not sure about this approach. Wouldn't it be a lot easier to:

  1. Move BaseTemplate to a separate module and make it an abstract class.
  2. Disallow Templates service to depend on it directly.
  3. Remove AMP.BaseTemplate binding intirely.
  4. Make implementations simply import BaseTemplate and extend it directly, instead of AMP.BaseTemplate.

If we do so, we can keep all code and APIs intact. This will end up just a very simple change.

@xiexr151e
Copy link
Contributor Author

Not sure about this approach. Wouldn't it be a lot easier to:

  1. Move BaseTemplate to a separate module and make it an abstract class.
  2. Disallow Templates service to depend on it directly.
  3. Remove AMP.BaseTemplate binding intirely.
  4. Make implementations simply import BaseTemplate and extend it directly, instead of AMP.BaseTemplate.

If we do so, we can keep all code and APIs intact. This will end up just a very simple change.

Can we move BaseTemplate into extensions?

@dvoytenko
Copy link
Contributor

@xiexr151e

Can we move BaseTemplate into extensions?

There are a few extensions that use BaseTemplate. So I'm proposing: we have a BaseElement in a shared location and all extensions compile it it directly. Thus v0.js will have no footprint of BaseTemplate.

@xiexr151e xiexr151e changed the title 🚀 Moving template-impl code to amp-mustache extension. 🚀 Separating BaseTemplate from template-impl. Sep 15, 2020
@xiexr151e xiexr151e marked this pull request as ready for review September 15, 2020 19:23
dvoytenko
dvoytenko suggested changes Sep 15, 2020
View reviewed changes
src/base-template.js Show resolved Hide resolved
src/base-template.js Show resolved Hide resolved
src/base-template.js Outdated Show resolved Hide resolved
xiexr151e
xiexr151e commented Sep 15, 2020
View reviewed changes
test/unit/test-template.js Outdated Show resolved Hide resolved
@xiexr151e
Copy link
Contributor Author

Hey @dvoytenko , could you clarify point 2 in the PR? I am currently running into a type error in which I am instantiating an abstract class - this did not happen when BaseTemplate was not abstract.

@xiexr151e
Copy link
Contributor Author

Pinging @ampproject/build-cop again for flaky test.

@zhouyx
Copy link
Contributor

zhouyx commented Sep 16, 2020

Could you provide more information on the flaky test? We currently have #30247 to deflake one of the test, but not sure that's the one which is blocking you.

@xiexr151e
Copy link
Contributor Author

Could you provide more information on the flaky test? We currently have #30247 to deflake one of the test, but not sure that's the one which is blocking you.

Based on the most recent cross-browser test, the test that failed was indeed a handshake test.

@xiexr151e xiexr151e merged commit b2bfc26 into ampproject:master Sep 19, 2020
@jridgewell
Copy link
Contributor

Sweet:

dist/v0.js: Δ -0.19KB

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@xiexr151e @ed-bird
🚀 Separating BaseTemplate from template-impl. (ampproject#30208)
a6350a2 
* initial attempt

* correction

* undo

* fix lgtm issue

* first attempt in moving template

* moved BaseTemplate to its own class

* missing changes

* uncomment existing test

* license header

* import

* comment out test

* lint

* type error

* undo abstract

* workaround

* lint

* typeof change

* conflict

* abs fix

Co-authored-by: Derek Tse <derektse@google.com>
@ampprojectbot ampprojectbot mentioned this pull request Jul 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinkimball kevinkimball kevinkimball left review comments

@dvoytenko dvoytenko dvoytenko approved these changes

@erwinmombay erwinmombay Awaiting requested review from erwinmombay

@jridgewell jridgewell Awaiting requested review from jridgewell

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xiexr151e @dvoytenko @zhouyx @jridgewell @kevinkimball @ampprojectbot