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

Migrate to x-data-grid v7 #5597

Merged
merged 11 commits into from
Jun 12, 2024
Merged

Migrate to x-data-grid v7 #5597

merged 11 commits into from
Jun 12, 2024

Conversation

LittleBigOwI
Copy link
Contributor

@LittleBigOwI LittleBigOwI commented May 24, 2024

Migrate to v7.5.1, from 6.19.11

Changes

  • Changed valueGetter and valueFormatter signatures as said in the MUI migration docs. MUI docs
  • Drop @mui/x-data-grid/legacy support. MUI docs

Fixes. #5524

@LittleBigOwI LittleBigOwI requested a review from a team as a code owner May 24, 2024 08:50
@LittleBigOwI
Copy link
Contributor Author

From my understanding, the es-check error comes from the @mui/x-data-grid library and not my code? Not sure.

@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented May 24, 2024

From my understanding, the es-check error comes from the @mui/x-data-grid library and not my code? Not sure.

In general, this means you need to add that library to the list so that it is compatible with the old devices we are targeting (mostly TVs).

But since the Dashboard isn't accessible from TV, perhaps we need to add an exception to ES-check (allow ES6 for the Dashboard). 🤔

@LittleBigOwI
Copy link
Contributor Author

How exactly would adding the library to the list make a difference in compatibility with TVs? Do older TVs not support ES6? Would putting the library in the list whitelist it from checks?

@dmitrylyzo
Copy link
Contributor

How exactly would adding the library to the list make a difference in compatibility with TVs?

The listed modules will be transpiled to ES2015.

Do older TVs not support ES6?

Yes. The lowest targets are Tizen 2.3 and webOS 1.2. They are from 2015 (old WebKit ~ Chrome 27).

Would putting the library in the list whitelist it from checks?

The list I mentioned is for babel-loader, i.e. transpilation.

For ES-check, it is

jellyfin-web/.escheckrc

Lines 5 to 10 in 2d46211

"not": [
"./dist/libraries/pdf.worker.js",
"./dist/libraries/worker-bundle.js",
"./dist/libraries/wasm-gen/libarchive.js",
"./dist/serviceworker.js"
]

I think it is safe to exclude Dashboard from ES-check. cc @thornbill

webpack.common.js Outdated Show resolved Hide resolved
@LittleBigOwI
Copy link
Contributor Author

Guess I'll wait for Bill before committing anything then. But is the only option to fix es-check to ignore the file completely like this? It seems that adding x-data-grid to the list of transpiled libraries doesn't fix the check. And if it does come to ignoring the library completely, I'm guessing that adding it to the list of transpiled libraries doesn't make much sense.

"not": [
    "./dist/libraries/pdf.worker.js",
    "./dist/libraries/worker-bundle.js",
    "./dist/libraries/wasm-gen/libarchive.js",
    "./dist/[email protected].*.js",
    "./dist/serviceworker.js"
]

@thornbill
Copy link
Member

I think it is safe to exclude Dashboard from ES-check.

Yeah I agree. We will probably need a separate build of the dashboard to fully realize this, but for this PR we should be good to just ignore the data grid since it isn't used elsewhere. 👍

@thornbill thornbill added the enhancement Improve existing functionality or small fixes label May 24, 2024
@thornbill thornbill added this to the v10.10.0 milestone May 24, 2024
@LittleBigOwI
Copy link
Contributor Author

All done! Thanks guys for being helpful 👍

@thornbill
Copy link
Member

@LittleBigOwI Thanks for the PR! Please don't keep adding merge commits. Unless there is a conflict these just add noise. Thanks again!

webpack.common.js Outdated Show resolved Hide resolved
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jun 10, 2024
@jellyfin-bot

This comment has been minimized.

.escheckrc Outdated Show resolved Hide resolved
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jun 11, 2024
Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@thornbill thornbill added the dependencies Pull requests that update a dependency file label Jun 12, 2024
@thornbill thornbill merged commit be386fb into jellyfin:master Jun 12, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement Improve existing functionality or small fixes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants