[core] Add missing test case for restricted-path-imports #20350
Conversation
Details of bundle changes.Comparing: 5aad5f2...d2fab72 Details of page changes
|
oliviertassinari
changed the title
| @@ -12,6 +12,7 @@ ruleTester.run('restricted-path-imports', rule, { | |||
| "import { blue } from '@material-ui/core/colors'", | |||
| "import * as colors from '@material-ui/core/colors'", | |||
| "import * as colors from '@another/core/styles/withStyles'", | |||
| "import describeConformance from '@material-ui/core/test-utils/describeConformance'", | |||
There was a problem hiding this comment.
Do we still use this path or why should this be valid?
There was a problem hiding this comment.
Ah not sure, I just copied this lint rule into my repo at work cos it's not published, and noticed this inconsistency.
There was a problem hiding this comment.
There was a problem hiding this comment.
Tricky. This was never intended to be a public module which is why it is not exported from test-utils/index. But let's leave it be for now (since it didn't cause us any trouble after we hid it in a deep import).
We should add a TODO notice that we this should fail in v5 where we'll move it to our internal /test/utils.
There was a problem hiding this comment.
Sorry, that's not what I meant - I copied and pasted the lint rule along with its tests (just in case ESLint 7 comes out and breaks it) into my repo to guard against deep imports in our codebase (I'm trying to make our UMD build depend on yours which means no deep imports and actually led me to #20353), and then as I was familiarising myself with your code & tests, I saw that the condition wasn't being tested :) We don't use this util in our tests, but I'm happy to add the TODO.
There was a problem hiding this comment.
We should add a TODO notice that we this should fail in v5 where we'll move it to our internal /test/utils.
@eps1lon I think that it could be interesting to have this module public, especially for third-party authors that want to enforce the same constraints than our components. For instance, if we didn't plan to move @material-ui/pickers into the mono-repository, it would have helped.
There was a problem hiding this comment.
Sure but this should be another package so that we can properly declare (peer) dependencies. Right now you have to be able to understand the source to be able to make it work. At that point you might as well inline it.
There was a problem hiding this comment.
@eps1lon Oh, so like a @material-ui/test package? Maybe it's simpler to make it private for now?
There was a problem hiding this comment.
As I said there's no gain now by making this private now. Let's keep this simple and mark this as undesired and strip out test-utils in v5. We can discuss what's useful in test-utils for other packages in a separate issue (and if this should be a "test" package).
There was a problem hiding this comment.
How would you like me to proceed with this?
- add a
TODOto the top of thedescribeConformancemodule - point 1 but also re-export it from
core/test-utils, update imports, and remove the exclusion from the ESLint rule
|
@oliviertassinari thanks for updating the PR title, I'll update the commit message too. |
Could you elaborate? What is off exactly, or why shouldn't it pass? |
|
@NMinhNguyen We decided to forbidden nested imports. So either we make this module private, or we import from the second level. |
|
In order not to accidentally break someone that could be relying on this (although it's already private), would you like me to re-export this module from |
This condition wasn't properly tested:
https://github.com/mui-org/material-ui/blob/396e4ef3cf3e13bafeac4d2019dd0771fe9f8ebf/packages/eslint-plugin-material-ui/src/rules/restricted-path-imports.js#L10-L11