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.

Sign up for GitHub

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

Jump to bottom

[core] Add missing test case for restricted-path-imports #20350

Merged
merged 1 commit into from
Apr 1, 2020
Merged

[core] Add missing test case for restricted-path-imports #20350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/eslint-plugin-material-ui/src/rules/restricted-path-imports.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Copy link
Member

Choose a reason for hiding this comment

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

Do we still use this path or why should this be valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah not sure, I just copied this lint rule into my repo at work cos it's not published, and noticed this inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think you do:

https://github.com/mui-org/material-ui/blob/4fba0dafd30f608937efa32883d151ba01fc9681/packages/material-ui-lab/src/Alert/Alert.test.js#L3

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@eps1lon Oh, so like a @material-ui/test package? Maybe it's simpler to make it private for now?

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you like me to proceed with this?

  1. add a TODO to the top of the describeConformance module
  2. point 1 but also re-export it from core/test-utils, update imports, and remove the exclusion from the ESLint rule

"import describeConformance from '@another/core/test-utils/describeConformance'",
],
invalid: [
Expand Down