Skip to content

🖍 Increase CSS byte limit from 50000 bytes to 75000 bytes#26475

Merged
kristoferbaxter merged 2 commits intoampproject:masterfrom
kristoferbaxter:increase-css-limit
Feb 8, 2020
Merged

🖍 Increase CSS byte limit from 50000 bytes to 75000 bytes#26475
kristoferbaxter merged 2 commits intoampproject:masterfrom
kristoferbaxter:increase-css-limit

Conversation

@kristoferbaxter
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter commented Jan 24, 2020 •
edited
Loading

Increase allowed css byte_limit in validation. This PR implements #26466

Please note: Haven't made changes in this section of the codebase before, a more thorough review is appreciated.

Edit: Incorrect # reference for implementation.

@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Jan 24, 2020

Hey @ampproject/wg-caching, these files were changed:

  • validator/validator-main.protoascii

@Gregable
Copy link
Copy Markdown
Member

Gregable commented Jan 24, 2020

cc @ampproject/wg-amp4email since this effects the email spec too.

Not sure who to cc on the amp actions side.

Gregable
Gregable approved these changes Jan 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gregable Gregable left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The validator change looks technically correct.

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Jan 24, 2020 •
edited
Loading

This PR implements #26094

@kristoferbaxter Isn't it actually implementing #26466 (and/or #4555)?

@westonruter westonruter mentioned this pull request Jan 24, 2020
Closed
@kristoferbaxter
Copy link
Copy Markdown
Contributor Author

kristoferbaxter commented Jan 24, 2020

Updated description, thanks @westonruter.

@nainar
Copy link
Copy Markdown
Contributor

nainar commented Jan 25, 2020

Will raise this as a discussion point at the next @ampproject/wg-amp4email meeting.

@kristoferbaxter
Copy link
Copy Markdown
Contributor Author

kristoferbaxter commented Jan 27, 2020

@nainar When is the next meeting? Considering splitting the email spec change to a separate PR.

@nainar
Copy link
Copy Markdown
Contributor

nainar commented Jan 28, 2020

The next meeting is on 13 Feb (PST timezone). We can make the Email change separately if the limits are exposed separately for sure.

@kristoferbaxter kristoferbaxter mentioned this pull request Jan 28, 2020
Closed
@nainar
Copy link
Copy Markdown
Contributor

nainar commented Feb 3, 2020

@ampproject/wg-approvers for approval.

cc @cramforce @dvoytenko @choumx

@kristoferbaxter can we pull the Email change out of this PR?

@dvoytenko
Copy link
Copy Markdown
Contributor

dvoytenko commented Feb 4, 2020

👍

rsimha
rsimha reviewed Feb 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

I found a few more mentions of the 50k limit:

amphtml/spec/email/amp-email-format.md

Line 99 in d6c7bae

Note: The entire `<style>` tag cannot exceed 50,000 bytes. The validator will check for this.

amphtml/validator/java/src/main/proto/validator.proto

Line 273 in eccff2d

// can potentially take the number of bytes over the 50,000 byte limit due

amphtml/validator/validator-main.protoascii

Line 1260 in eccff2d

max_bytes: 20000 # Smaller than AMP, which is 50,000.

amphtml/validator/validator.pb.go

Line 1889 in eccff2d

// can potentially take the number of bytes over the 50,000 byte limit due

amphtml/validator/validator.proto

Line 270 in eccff2d

// can potentially take the number of bytes over the 50,000 byte limit due

@kristoferbaxter
Copy link
Copy Markdown
Contributor Author

kristoferbaxter commented Feb 4, 2020

Thanks Raghu. I’ll add the appropriate items into this PR.

@kristoferbaxter
Copy link
Copy Markdown
Contributor Author

kristoferbaxter commented Feb 7, 2020

@CrystalOnScript

Can you approve the markdown change so we can move forward with this commit?

@nainar
Copy link
Copy Markdown
Contributor

nainar commented Feb 7, 2020

Chatted with @ianba for AMP Actions and they don't have a preference between:

  1. Stay the same at 50k OR
  2. Increase to 75k.

So feel free to do whichever is easier.

@kristoferbaxter
Copy link
Copy Markdown
Contributor Author

kristoferbaxter commented Feb 7, 2020

Thanks Naina!

@kristoferbaxter kristoferbaxter merged commit 489204f into ampproject:master Feb 8, 2020
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Feb 8, 2020

Now that this has been merged, is it right that the it would be released as stable with changes deployed to the validator on the Tuesday after next? In other words, would this be live on February 18th?

@Gregable
Copy link
Copy Markdown
Member

Gregable commented Feb 8, 2020

We'll update the thread when it gets released.

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Feb 8, 2020

OK, just looking to get an idea for when this will be live so that we can ensure the AMP WordPress plugin is updated as soon as possible to take advantage of the increased limit.

@Gregable
Copy link
Copy Markdown
Member

Gregable commented Feb 8, 2020

Your timeline is the right order of magnitude, but since this constant has never changed, there may be some internal tooling (updating test goldens and so forth) the AMP Cache needs to do when this gets pulled in which could delay release. Therefore, I don't want to commit to that timeline.

honeybadgerdontcare added a commit that referenced this pull request Feb 12, 2020
@amaltas @honeybadgerdontcare
Validator rollup (#26771)
d4800ac 
* cl/294457507 Revision bump for #26475

* cl/294516530 Revision bump for #26708

* cl/294516739 Update node.js version numbers

* cl/294681603 Revision bump for #26747

Co-authored-by: honeybadgerdontcare <honeybadgerdontcare@users.noreply.github.com>
amaltas added a commit that referenced this pull request Feb 12, 2020
@amaltas @honeybadgerdontcare
Update nodejs and gulpjs versions and README (#26774)
ac90be1 
* cl/294457507 Revision bump for #26475

* cl/294516530 Revision bump for #26708

* cl/294516739 Update node.js version numbers

* cl/294681603 Revision bump for #26747

* Update package versions and README for nodejs and gulpjs. (Packages are
pushed  to npm repository)

Co-authored-by: honeybadgerdontcare <honeybadgerdontcare@users.noreply.github.com>
@patrickkettner patrickkettner mentioned this pull request Feb 14, 2020
Merged
@westonruter westonruter mentioned this pull request Feb 18, 2020
Merged
3 tasks
Cauchon added a commit to Cauchon/amp.dev that referenced this pull request Feb 24, 2020
@Cauchon
Updated CSS limit to 75,000 bytes from 50,000
504ba6e 
ampproject/amphtml#26475
@Cauchon Cauchon mentioned this pull request Feb 24, 2020
Merged
Cauchon added a commit to Cauchon/amp.dev that referenced this pull request Feb 24, 2020
@Cauchon
Updating page to reflect updated CSS limit
ee882d0 
https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/

The CSS limit has been upped to 75,000 per ampproject/amphtml#26475
@Cauchon Cauchon mentioned this pull request Feb 24, 2020
Merged
@patrickkettner patrickkettner mentioned this pull request Feb 24, 2020
Merged
CrystalOnScript pushed a commit to ampproject/amp.dev that referenced this pull request Feb 25, 2020
@Cauchon
Updated CSS limit to 75,000 bytes from 50,000 (#3598)
b80dab9 
ampproject/amphtml#26475
matthiasrohmer pushed a commit to ampproject/amp.dev that referenced this pull request Feb 25, 2020
@Cauchon
Updating page to reflect updated CSS limit (#3599)
793e640 
https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/

The CSS limit has been upped to 75,000 per ampproject/amphtml#26475
@Gregable
Copy link
Copy Markdown
Member

Gregable commented Feb 26, 2020

This has been rolled out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gregable Gregable Gregable approved these changes

@CrystalOnScript CrystalOnScript CrystalOnScript approved these changes

+1 more reviewer

@rsimha rsimha rsimha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes PR use: In Beta / Experimental PR use: In Stable WG: amp4email WG: caching WG: outreach WG: performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@kristoferbaxter @Gregable @westonruter @nainar @dvoytenko @CrystalOnScript @rsimha @googlebot @ampprojectbot