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

[core] Add eslint rule to restrict import from ../internals root #13633

Merged

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Jun 26, 2024

Following the discussion in #13562 where valid points were raised regarding folder structure and our own conventions, it made more sense to address the actual issue in the charts codebase, where the ../internals root is sometimes automatically imported by the IDE, which has a high probability of causing cyclic imports.

All packages?

This rule be updated to prevent this issue for all packages if needed.
Main issues when using on all our packages.

"/mui-x/packages/x-data-grid/src/components/cell/GridCell.tsx"
  10:39  error  Unexpected path "../../internals" imported in restricted zone  import/no-restricted-paths

"/mui-x/packages/x-date-pickers/src/DateTimePicker/DateTimePickerToolbar.tsx"
  23:39  error  Unexpected path "../internals" imported in restricted zone  import/no-restricted-paths

"/mui-x/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.tsx"
  37:8  error  Unexpected path "../internals" imported in restricted zone  import/no-restricted-paths

"/mui-x/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePickerLayout.tsx"
  14:44  error  Unexpected path "../internals" imported in restricted zone  import/no-restricted-paths

@JCQuintas JCQuintas added the core Infrastructure work going on behind the scenes label Jun 26, 2024
@JCQuintas JCQuintas self-assigned this Jun 26, 2024
@mui-bot
Copy link

mui-bot commented Jun 26, 2024

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

Generated by 🚫 dangerJS against 9dfaa6a

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Good for charts. I let others say if they want the rule applied to their packages too

@flaviendelangle
Copy link
Member

flaviendelangle commented Jun 26, 2024

100% in favor of doing it for the Tree View and the Pickers

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 change. 💯
I think that it would be a nice improvement to other packages as well. 👍

@JCQuintas
Copy link
Member Author

@LukasTy @flaviendelangle added the treeview and pickers packages as well, let me know if the adapted code is correct 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants