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

New transformer to convert ".js" to ".js?f=sxg" in script node values. #451

Closed
wants to merge 31 commits into from

Conversation

xiexr151e
Copy link

@xiexr151e xiexr151e commented Jul 11, 2020

This transformer will be used as part of creating the SxG build for AMP, based off of the module build.

@xiexr151e
Copy link
Author

@twifkak @erwinmombay

@erwinmombay erwinmombay requested review from twifkak and banaag July 13, 2020 21:22
@honeybadgerdontcare
Copy link
Collaborator

Is there an issue describing what situation this is addressing?

@xiexr151e
Copy link
Author

Is there an issue describing what situation this is addressing?

@honeybadgerdontcare I don't believe so. These are just part of a series of changes needed to make as part of preparing for a SxG build for AMP.

Copy link
Member

@twifkak twifkak left a comment

Choose a reason for hiding this comment

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

Thanks for this change! I haven't taken a look at the code in detail yet, but I think a few things need to be done before we can land this PR:

  • Serve .sxg.js for v0 and all the extensions at cdn.ampproject.org. (edit: solved with the change to ?f=sxg thought the contents are currently equal)
  • Change cdn.ampproject.org cache transformations to rewrite .sxg.js back to .js for serving in AMP Viewer (edit: solved with http://cl/324144375)
  • Change validator (either protoascii, or C++&JS) to allow these URLs in transformed AMP (edit: not yet solved)

There are enough users of the master branch that I wouldn't want to introduce a known breakage.

@honeybadgerdontcare @banaag My understanding is that this is the next step in differential loading of JS beyond module/nomodule. They're planning to ship a version of the JS that has viewer integration code and post-module polyfills removed so it has a smaller footprint.

@xiexr151e It might be worth a quick doc to explain the problem/solution. That would more easily allow multiple threads of discussion, and serve as a unified location for the overall plan.

CODE_OF_CONDUCT.md Show resolved Hide resolved
transformer/transformers/ampruntimejs_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Can someone point me to a doc explaining why we need .sxg.js?

@xiexr151e
Copy link
Author

@twifkak @honeybadgerdontcare @jridgewell @erwinmombay @kevinkimball
Since we have already discussed this, I've updated the transformer accordingly to reflect our changes. Could you please take another look?

@erwinmombay
Copy link
Member

@xiexr151e can you update the subject of this PR as it seems obsolete

@erwinmombay
Copy link
Member

also lets externalize our design document and link it here for posterity

@xiexr151e xiexr151e changed the title New transformer to convert ".js" to ".sxg.js" in script node values. New transformer to convert ".js" to ".js&f=sxg" in script node values. Aug 12, 2020
@xiexr151e xiexr151e changed the title New transformer to convert ".js" to ".js&f=sxg" in script node values. New transformer to convert ".js" to ".js?f=sxg" in script node values. Aug 12, 2020
transformer/transformer.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs_test.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs_test.go Outdated Show resolved Hide resolved
Copy link
Member

@twifkak twifkak left a comment

Choose a reason for hiding this comment

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

Hi @xiexr151e! Thanks for the updates. Looks like 2 of the 3 blockers are solved, but this is still not valid transformed AMP. Those changes need to be made (and deployed within the Google AMP Cache) before we can merge this. Do you or @erwinmombay own the task of fixing the validator?

transformer/transformers/ampruntimejs.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs.go Outdated Show resolved Hide resolved
transformer/transformers/ampruntimejs_test.go Show resolved Hide resolved
transformer/transformers/ampruntimejs.go Outdated Show resolved Hide resolved
@xiexr151e
Copy link
Author

Hi @xiexr151e! Thanks for the updates. Looks like 2 of the 3 blockers are solved, but this is still not valid transformed AMP. Those changes need to be made (and deployed within the Google AMP Cache) before we can merge this. Do you or @erwinmombay own the task of fixing the validator?

We jointly own this task, but I will be the primary assignee to the task. We are scheduled to begin the task next week.

@erwinmombay erwinmombay self-assigned this Sep 25, 2020
@CLAassistant
Copy link

CLAassistant commented Oct 8, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 5 committers have signed the CLA.

✅ twifkak
✅ banaag
✅ nainar
❌ xiexr151e
❌ Derek Tse


Derek Tse seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@erwinmombay
Copy link
Member

going to close this. The SxG project is still on going but I will pick it up next quarter. and the code here is already obsolete

@erwinmombay erwinmombay closed this Feb 8, 2021
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.

9 participants