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

[DataGridPremium] Namespace Excel export worker #16020

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 28, 2024

The current logic looks wrong. Workers share the same pub-sub channel namespace. I believe it's why, e.g. https://revealjs.com/postmessage/ has a namespace property. You can reproduce a conflict by running

pnpm test:regressions:dev

and opening the page:

SCR-20241228-tvwf

I don't know how to write a test for this effectively, so I skipped it. I initially noticed this improving slightly the visual regression flow in Material UI, saw an error in the console, which shouldn't be there.

Preview: https://deploy-preview-16020--material-ui-x.netlify.app/x/react-data-grid/export/#using-a-web-worker

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! plan: Premium Impact at least one Premium user feature: Export labels Dec 28, 2024
@mui-bot
Copy link

mui-bot commented Dec 28, 2024

Deploy preview: https://deploy-preview-16020--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 7138f2f

@zannager zannager requested a review from cherniavskii January 22, 2025 10:56
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 11, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@cherniavskii cherniavskii changed the title [data grid] Namespace Excel export worker [DataGridPremium] Namespace Excel export worker Feb 11, 2025
@cherniavskii cherniavskii added needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Feb 11, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 11, 2025
@cherniavskii cherniavskii merged commit 3262a7a into mui:master Feb 11, 2025
18 checks passed
@mui mui deleted a comment from github-actions bot Feb 11, 2025
@mui mui deleted a comment from github-actions bot Feb 11, 2025
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

@oliviertassinari
Copy link
Member Author

Ah, I had forgotten about those. I have open like 40 PRs during the chrismas break but I had no time to work on those since. Thanks

@cherniavskii
Copy link
Member

I have open like 40 PRs during the chrismas break

Very productive holidays! 😅
Thanks for spotting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Export needs cherry-pick The PR should be cherry-picked to master after merge plan: Premium Impact at least one Premium user v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants