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

[docs-infra] Fix crawler on API pages #39490

Merged
merged 7 commits into from
Oct 31, 2023

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Oct 17, 2023

Due to the new API page #38265, the API layout is randomly chosen the first time a user connects. This choice is saved in the localStorage. But crawlers like Algolia don't keep that information, so at each crawl, they could face either the table, collapse, or expanded layout.

To write a craw for only one layout, we need to enforce this layout when Algolia is visiting. This modification enforces the expanded layout for Algolia and Google bots

Fix https://groups.google.com/a/mui.com/g/docs-infra/c/cr9i8LDORjE

Preview https://deploy-preview-39490--material-ui.netlify.app/material-ui/api/autocomplete/#Autocomplete-prop-autoHighlight

@alexfauquette alexfauquette added the scope: docs-infra Specific to the docs-infra product label Oct 17, 2023
@mui-bot
Copy link

mui-bot commented Oct 17, 2023

Netlify deploy preview

https://deploy-preview-39490--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 59b3bc3

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 17, 2023
@alexfauquette
Copy link
Member Author

Seems to work. I can't try the algolia on the preview since we are not allowed to crawl netlify (make sens)

But If I clean the local storage and set the user agent, I always get the expected layout 👍

image

@alexfauquette alexfauquette requested a review from a team October 17, 2023 16:20
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It looks good to me.

Side notes:

  1. We use a useEffect to toggle the layout. A useLayoutEffect (useEnhancedEffect) would be better (still doing the React rendering twice but not the DOM updates twice):
Screen.Recording.2023-10-17.at.17.52.21.mov
  1. Should we send Google Analytics? Or is the idea to have this toggle long-term?
  2. If we want to have it long-term, I think it would be nice to anchor in the anchor link in which the view is used, or maybe keep it simple, with having a single view map to the anchor links, like the one in the middle server-side, as well as when an anchor link is used. There are cases right now where the anchor link doesn't work.

@alexfauquette
Copy link
Member Author

There are cases right now where the anchor link doesn't work

That should not exist. anchor should work with all of them since they are also used in the right menu

Should we send Google Analytics? Or is the idea to have this toggle long-term?

It's already sent. Still need to configure the GA to get those user attributes into a nice table that match their initial layout to the one they chose

Currently on the dev environement:

image

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 17, 2023

It's already sent

@alexfauquette Oh, I see, it's set as a user property on the first page load.

Is this enough? Maybe we need to collect this between Next.js page navigations or maybe as frequently as each time the display changes. I guess we will see, we can first ship and measure.

That should not exist anchor should work with all of them since they are also used in the right menu

A reproduction:

Screen.Recording.2023-10-18.at.00.40.17.mov

@alexfauquette
Copy link
Member Author

Is this enough? Maybe we need to collect this between Next.js page navigations or maybe as frequently as each time the display changes. I guess we will see, we can first ship and measure.

The ideal solution would be to update user properties each time a user update its layout. But that would imply to create a ContextProvider such that the analytics can get an update when a property is modified.

I think people will pick the layout they prefer, so after a week most of our user should have a property defined, and we will be able to see if their is a clear winner or not

A reproduction:

Ok, I thought it was an issue with ids miss aligned. But it's slightly more complex.

The server side element is the collapsed layout. So when you provide a hash, it scrolls up to the item according to the collapsed layout. Then we update the layout to the one you chose, but the scroll stay where it was

I'm pushing a fix to:

  • set expanded as the default such that Algolia and Google get the correct page from server side rendering without having to run JS
  • add a useEnhancedEffect to scroll to the correct location after hydratation

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The ideal solution would be to update user properties each time a user update its layout. But that would imply to create a ContextProvider such that the analytics can get an update when a property is modified.

I was thinking of calling window.gtag from inside ToggleDisplayOption.tsx.

We could move some of the logic that is inside GoogleAnalytics.js closer to where the state is defined (the useState origin of useNoSsrCodeStyling, useUserLanguage, useNoSsrCodeVariant are).

But in any case, I agree, one trigger is already a good start. After a week, we will see how the users with a repeat page loads behave, it might be enough.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Currently on the dev environement:

Ah nice, we can make the report public once ready 👌

Screenshot 2023-10-19 at 15 34 21

@alexfauquette
Copy link
Member Author

alexfauquette commented Oct 23, 2023

To fix the server-side rendering I thought about using cookies as a storage such that the page could be dynamically rendered depending on the cookies settings. But using cookies() leads to the following error message. I suspect it only works in the app folder since it requires the notion of a server component

Error: Invariant: cookies() expects to have requestAsyncStorage, none available.

We can't use the ReactDOM.flushSync in a useEffect or useLayoutEffect. It can only be run outside of a render cycle. For example during an event listener. Maybe it could be used after the page-loaded event.

But for now, it seems that the use of useLayoutEffect instead of useEffect is enough for the scrolling when reiredt, but not when loading 🤔

Screencast.from.23-10-2023.13.08.33.mp4

@alexfauquette
Copy link
Member Author

To have it work, I ended up using an intermediate state needsScroll to let the updated layout rendering occur before the scroll.

…on.tsx

Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the changes. Google's market dominance tends to make us focus in on them specifically. A decade of building web crawlers taught me there are more actors interested in the bot experience.

On a side note, it's the same experience that taught me that random content on page load tends to make things harder than they should be. When it comes to webpages, non-determinism really adds a layer of complexity that tends to contain hard to debug issues. I'd always avoid it if I could. If there are no hard product requirements to have this random behavior, I'd be happy to help brainstorm alternatives that offer the same outcome, but keeps page load deterministic.

@alexfauquette
Copy link
Member Author

When it comes to webpages, non-determinism really adds a layer of complexity that tends to contain hard to debug issues. I'd always avoid it if I could.

The idea is that we have multiple layouts for the API page and no idea about which one is the best. So we are trying them by assigning a random one to each user the first time they arrive. If after a month we get a clear winner, we could consider using it as the default for new comers.

@alexfauquette alexfauquette merged commit d585bb8 into mui:master Oct 31, 2023
19 checks passed
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 29, 2023

If we now have enough data, I think it would be great to remove the random picker. It creates a layout shift, not a great UX https://groups.google.com/a/mui.com/g/docs-infra/c/bzqriS4u3_M

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants