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

Add back no-restricted-modules and no-restricted-imports ESLint rules #12446

Merged
merged 1 commit into from
Mar 23, 2017

Conversation

oandregal
Copy link
Contributor

They were removed in #6945 inadvertently.

@matticbot
Copy link
Contributor

@oandregal oandregal self-assigned this Mar 23, 2017
@oandregal oandregal requested a review from tyxla March 23, 2017 09:00
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Nice, it's a good idea to have these added back.

However, I find something problematic here. If we set these as errors, we'll be forced to migrate away from sites-list in any file that currently includes it - which we can't do right now. Migration of sites-list still requires a lot of work, part of it has already been started and can be monitored in the dedicated project: https://github.com/Automattic/wp-calypso/projects/3

So, I'd recommend setting these to be warnings instead. Once we're able to safely get rid of sites-list, we can change these back to errors.

@oandregal
Copy link
Contributor Author

The existing code that uses sites-list doesn't need to be migrated with this change, eslines will downgrade it to a warning for the CI/hooks not to complain.

My understanding is that we don't allow using those modules for any new code, that's where they'll be reported as errors. If that assumption holds false, we may set them as warnings or remove the rules - either way is fine with me, although I'd lean towards removing them.

@tyxla
Copy link
Member

tyxla commented Mar 23, 2017

eslines will downgrade it to a warning for the CI/hooks not to complain.

Ah, I guess I should have got to know eslines better. Well, if that's the case, this LGTM 👍

@oandregal oandregal merged commit ea68492 into master Mar 23, 2017
@oandregal oandregal deleted the fix/eslint-add-back-no-restricted branch March 23, 2017 09:50
@matticbot matticbot added [Size] S Small sized issue and removed [Status] Ready to Merge labels Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework [Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants