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] Cover webpack 4 support in migration guide #12710

Merged
merged 6 commits into from Apr 22, 2024

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Apr 8, 2024

@cherniavskii cherniavskii added the docs Improvements or additions to the documentation label Apr 8, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice initiative! 👏
It's yet another section that would be a great addition to the overall v7 migration guide. 🙈
IMHO, if we keep using separate pages, then this section has to end up at least in Pickers page as well. 🤔
Technically, it makes sense in all the migration guides, but tree-view and charts might not be that relevant as there are probably close to zero users on legacy setups. 🙈

@cherniavskii
Copy link
Member Author

IMHO, if we keep using separate pages, then this section has to end up at least in Pickers page as well. 🤔

Yes, I plan to add a similar section for the pickers and tree view as well.
I want to get some feedback on #12612 (comment) first.

@flaviendelangle
Copy link
Member

Technically, it makes sense in all the migration guides, but tree-view and charts might not be that relevant as there are probably close to zero users on legacy setups.

For the Tree View is has been on the lab for years and the migration to X is super easy (more than a major from the grid probably).
So we probably have very old setups that are being migrated, just like the grid

Same for pickers

For the Charts, it's probably a lot less common since it's a brand new codebase

@cherniavskii
Copy link
Member Author

Covered the Webpack 4 in all 4 migration guides: data grid, pickers, tree view and charts 👍🏻

@cherniavskii cherniavskii marked this pull request as ready for review April 19, 2024 18:16
+ {
+ test: path.resolve(__dirname, 'node_modules'),
+ exclude: [
+ path.resolve(__dirname, 'node_modules/@mui/x-data-grid'),
Copy link
Member

Choose a reason for hiding this comment

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

I would be more explicit here. I guess nobody wants to do this to the license and the community package, they always want either to add some commercial package or skip the license.

I see 4 potential approaches:

  • just add a sentence (the one I wrote is quite bad)

    Suggested change
    + path.resolve(__dirname, 'node_modules/@mui/x-data-grid'),
    + // Add all folder of each grid packages you are using.
    + path.resolve(__dirname, 'node_modules/@mui/x-data-grid'),
  • list the 3 cases (@mui/x-data-grid | @mui/x-data-grid + @mui/x-data-grid-pro + @mui/x-license | @mui/x-data-grid + @mui/x-data-grid-pro + @mui/x-data-grid-premium | @mui/x-license)

  • list all the packages but add a small // (only needed for pro and premium users) and // (only needed for pro users) after the path.resolve(...) of each commercial package.

  • list all the packages even if they are not useful, I guess they do no harm and make sure people don't have nasty bugs if they buy a license in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

If they only use the community package, the license package won't be installed, so there's no harm in having it there.
I would rather keep the universal snippet that would work for every plan.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather keep the universal snippet that would work for every plan.

But do we agree that your snippet do not include the pro and premium packages and that can be problematic?
Or does node_modules/@mui/x-data-grid also match node_modules/@mui/x-data-grid-pro? In which case maybe just a comment saying that it does would be interesting.

Basically, just by reading the code snippet, the first thing I'd like to do as a pro user is to add -pro on the grid package line, and that is wrong because it will stop transpiling the community package, which is always needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

But do we agree that your snippet do not include the pro and premium packages and that can be problematic?
Or does node_modules/@mui/x-data-grid also match node_modules/@mui/x-data-grid-pro?

I tested the Pro and Premium packages with this snippet and it worked.
I will check again to make sure it does work, and add a comment if that's the case 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM, apart from the open discussion. 👍

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of it!

@cherniavskii cherniavskii merged commit 1f25e1f into mui:master Apr 22, 2024
15 checks passed
@cherniavskii cherniavskii deleted the update-migration-guide branch April 22, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation v7.x migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Cannot transpile since latest update v7.0.0
4 participants