Skip to content

✨amp-experiment 1.0: Added a 15KB limit on amp-experiment JSON#22372

Merged
torch2424 merged 1 commit intoampproject:masterfrom
torch2424:amp-experiment-json-limit
May 20, 2019
Merged

✨amp-experiment 1.0: Added a 15KB limit on amp-experiment JSON#22372
torch2424 merged 1 commit intoampproject:masterfrom
torch2424:amp-experiment-json-limit

Conversation

@torch2424
Copy link
Copy Markdown
Contributor

@torch2424 torch2424 commented May 17, 2019

relates to #20225
relates to #21705
relates to #22168

This adds a 15KB limit to the amp-experiment JSON. See the discussion in #22168 for the reasoning. It Was decided offline to add an additional 5KB, since different variants will be input that don't affect the mutation limit. 😄

This is done all through the validator, and updated the test file accordingly 👍

cc @Gregable for the validator review
cc @zhouyx for the "concept" review haha! 😂

@torch2424 torch2424 requested review from Gregable and zhouyx May 17, 2019 22:56
@Gregable
Copy link
Copy Markdown
Member

This looks as though it has the potential to break existing pages with >15KB on them already. Has anyone yet looked to see if that's a potential risk, or do you need me to run that analysis? (If the latter, this will be next week).

@torch2424
Copy link
Copy Markdown
Contributor Author

@Gregable Thank you for bringing this up! 😄

Has anyone yet looked to see if that's a potential risk, or do you need me to run that analysis?

Could we run the analysis if you don't mind? This isn't super high priority or anything, so next week is fine 😄 👍

Thanks!

@torch2424
Copy link
Copy Markdown
Contributor Author

@Gregable Confirmed this should not break anyone (Data is from last month though). But since the old JSON format wasn't very verbose, I think we are fine.

This is good for another review 😄

@torch2424
Copy link
Copy Markdown
Contributor Author

@zhouyx Says I don't need their approval as well 😂

Merging... 😄 👍

Thanks! @Gregable

@torch2424 torch2424 merged commit a50dcd7 into ampproject:master May 20, 2019
@torch2424 torch2424 deleted the amp-experiment-json-limit branch May 20, 2019 23:46
alin04 added a commit to alin04/amphtml that referenced this pull request May 21, 2019
@alin04 alin04 mentioned this pull request May 21, 2019
alin04 added a commit that referenced this pull request May 21, 2019
* cl/248381896 Revision bump for #22289

* cl/249156543 Allow amp-script with an experiment token.

* cl/249265307 Revision bump for #22372
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