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 Eslint rules to throw error when importing other modules from within modules directory. #3970

Closed
wants to merge 1 commit into from

Conversation

Fawke
Copy link
Contributor

@Fawke Fawke commented Jul 5, 2019

Type of change

  • Feature

Description of change

Related to #3964.

@Fawke Fawke mentioned this pull request Jul 5, 2019
@jsnellbaker jsnellbaker self-requested a review July 8, 2019 13:08
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@Fawke

This does look good overall, though I'm not sure about the exceptions made for the prebidServer and userId sub-folders. Can we restructure the logic to still allow the rules there? I get it was done to allow the config file to be imported, but I'm curious if there's another way to go about this.

@snapwich
Copy link
Collaborator

snapwich commented Jul 8, 2019

Just noticed this pull-request. I made a similar stab at it in #3976

If we can get public eslint plugins to work through configuration I think that's preferable; not sure if it's possible though so I ended up making a custom plugin. Some of things I wanted were:

  • a whitelist of modules allowed per directory paths (as opposed to a blacklist, or restrictions as you have here)), it might be possible with eslint-plugin-import but I couldn't get it working quite right.
  • validate that modules can be resolved and are not module entry points. The first part can be handled with eslint-plugin-import but I'm not sure about the later part.

If you can get those constraints working with this pull-request then I'm all for it, I'm not sure no-restricted-imports and no-restricted-modules are the rules we want though.

@jaiminpanchal27
Copy link
Collaborator

Closing this PR in favour of #3976

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants