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

DocumentHead: Remove refreshHeadTags() method #25003

Merged
merged 1 commit into from
Jun 15, 2018

Conversation

ockham
Copy link
Contributor

@ockham ockham commented May 22, 2018

Found while working on #24980. I read through #15297 which introduced this but couldn't make sense of why this method was introduced. (Most of all, adding meta and link tags on the client side doesn't seem to make a lot of sense when they're already added on the server side.) I'd be thankful if someone could explain the motivation for adding these methods.

My current strategy for fixing SSR is found in #24977. Since SSR is broken anyway right now, I don't assume this PR to have any impact.

Testing instructions: Make sure that Calypso works as before.

@ockham ockham added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 22, 2018
@ockham ockham self-assigned this May 22, 2018
@matticbot
Copy link
Contributor

@ockham ockham requested review from Tug, a team, stephanethomas and sirreal May 22, 2018 16:11
@Tug
Copy link
Contributor

Tug commented May 22, 2018

It does indeed look like those tags are not rendered anymore, so I don't have any problem with merging this 👍

It's weird that we allowed those tags to not be rendered at some point. Last time we did the switch to redirect /wp-login.php to /log-in we got an immediate ping from System about some internal checks that were failing.

@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 22, 2018
@ockham ockham force-pushed the remove/document-head-client-side-code branch from d9532d8 to b0ec732 Compare May 22, 2018 21:12
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Smoke tested a bit, seems to work well.

Code also looks great, nice cleanup 👍 💯 I would only probably remove the unused selectors before shipping 🔪

@@ -131,8 +95,6 @@ DocumentHead.propTypes = {
export default connect(
state => ( {
formattedTitle: getDocumentHeadFormattedTitle( state ),
allLinks: getDocumentHeadLink( state ),
Copy link
Member

Choose a reason for hiding this comment

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

These are the only usages of those 2 selectors. We could totally delete them as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err nope 😄

metas = getDocumentHeadMeta( context.store.getState() );
links = getDocumentHeadLink( context.store.getState() );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want these in our server-generated HTML, after all 😉

@ockham ockham force-pushed the remove/document-head-client-side-code branch from b0ec732 to 04f01dd Compare June 15, 2018 11:34
@ockham ockham merged commit 3350ed3 into master Jun 15, 2018
@ockham ockham deleted the remove/document-head-client-side-code branch June 15, 2018 12:11
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 15, 2018
@dmsnell dmsnell added the [Type] Bug When a feature is broken and / or not performing as intended label Jun 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants