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] Fix crash on Safari because of unsupported lookahead feature #30345

Merged
merged 7 commits into from
Dec 27, 2021

Conversation

cherniavskii
Copy link
Member

Fixes #30344

I've followed steps in described in Review suggestion #30107 to run test:e2e-website:dev, but relevant tests kept failing for some reason (even before my changes).
Let me know if there's something missing in the PR.

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 21, 2021

No bundle size changes

Generated by 🚫 dangerJS against e9d2c02

@oliviertassinari oliviertassinari changed the title [docs] fix crash on Safari because of unsupported lookahead feature [docs] Fix crash on Safari because of unsupported lookahead feature Dec 21, 2021
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work docs Improvements or additions to the documentation website Pages that are not documentation-related, marketing-focused. labels Dec 21, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 21, 2021

I have tried to push the approach further, describing why we have this logic. From what I understand, from an end-user perspective, to mitigate for the first 60 minutes that the crawl index is not up to date, which will lead to people having a 301. From a maintainer perspective, it's about being able to use the search with this feature flag enable.

@oliviertassinari oliviertassinari mentioned this pull request Dec 21, 2021
2 tasks
@oliviertassinari oliviertassinari added the regression A bug, but worse label Dec 22, 2021
@oliviertassinari
Copy link
Member

@siriwatknp Are you happy with this fix?

@siriwatknp
Copy link
Member

siriwatknp commented Dec 24, 2021

@cherniavskii @oliviertassinari thank you so much for the fix. However, I decided to change the migration strategy to another way without replacing the href return from the search result due to the work that I need to do on mui-x repo.

I am closing this PR and remove the logic in #30386

@siriwatknp siriwatknp closed this Dec 24, 2021
@oliviertassinari
Copy link
Member

@siriwatknp Will #30386 be merged before the next release? If there is a chance that it won't, I would suggest that we have a standalone PR to avoid a regression in production once we release HEAD. Currently, HEAD is broken, and production has a hotfix.

@siriwatknp
Copy link
Member

we have a standalone PR to avoid a regression

@oliviertassinari good point, maybe reopen and continue on this PR is better.

@siriwatknp siriwatknp reopened this Dec 24, 2021
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for the fix! I have added more tests.

@siriwatknp siriwatknp merged commit e2467d0 into mui:master Dec 27, 2021
@oliviertassinari
Copy link
Member

@siriwatknp Perfect thanks

@cherniavskii cherniavskii deleted the fix-safari-crash branch January 11, 2022 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation regression A bug, but worse website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mui.com crash on Safari
4 participants