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

core(fr): convert tags-blocking-first-paint gatherer #12527

Merged
merged 5 commits into from
May 20, 2021

Conversation

adamraine
Copy link
Member

Ref #11313

@adamraine adamraine requested a review from a team as a code owner May 20, 2021 16:11
@adamraine adamraine requested review from connorjclark and removed request for a team May 20, 2021 16:11
@google-cla google-cla bot added the cla: yes label May 20, 2021
@@ -82,7 +83,7 @@ async function collectTagsThatBlockFirstPaint() {
};
});

/** @type {Array<ScriptTag>} */
/** @type {ScriptTag[]} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

one step at a time... 🎉

Copy link
Member

Choose a reason for hiding this comment

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

🙄 the existing line is five months old. The decision we made was in part to prevent each person who touches a file doing a mass switch so it doesn't flip back and forth every five months. I'm fine rediscussing this (again), but as it is, this is kind of frustrating.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to agree. I mean changing them when you're already rewriting a block of code is one thing, but these are just completely untouched defs that are being changed here exclusively for preference.

If it's a war you want...

Copy link
Collaborator

@connorjclark connorjclark May 20, 2021

Choose a reason for hiding this comment

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

+1 to above points, lets revert. i was just jestin'

Copy link
Member Author

Choose a reason for hiding this comment

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

In my defense I had a function _getArtifact which used [] and wanted the rest of the file to match. I ended up removing the function so I'm fine reverting.

In the future, should I just adhere to whatever preference is already in the file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future, should I just adhere to whatever preference is already in the file?

Probably worth discussing in eng sync to make sure we all understand the same thing, but my understanding of where we landed was use whichever you prefer in new code you (re)write, leave all others untouched and unmentioned in review.

Totally no malice interpreted from this btw, consistency is a great reason to clean up code that you find, it's just that we've already abandoned the consistency argument for these 😉

@adamraine adamraine merged commit 04cc1d8 into master May 20, 2021
@adamraine adamraine deleted the fr-tags-blocking-first-paint branch May 20, 2021 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants