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

Implement new rule: no-internal-modules #485

Merged
merged 9 commits into from
Sep 15, 2016
Merged

Implement new rule: no-internal-modules #485

merged 9 commits into from
Sep 15, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Aug 12, 2016

See doc page for details

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.05%) to 97.721% when pulling 8a96faf on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.05%) to 97.721% when pulling f072e8a on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.05%) to 97.721% when pulling 4e9a2a1 on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.06%) to 97.722% when pulling 7c3ea3d on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.06%) to 97.722% when pulling 275ddc8 on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.06%) to 97.726% when pulling a2dd4fd on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@benmosher
Copy link
Member

Ah, sounds cool. Normally I'd like to see an issue with a proposal first, but a PR makes for a very explicit proposal. 😄

Adding minimatch is a non-trivial dependency, as other settings/options may leverage some sort of glob evaluator, and they should all use the same one. It's worth discussing what (if any) glob parsers are used for other plugins / ESLint core (i.e. the .eslintignore file must use something).

Need to get @jfmengels' thoughts on this, too.

@sindresorhus, I think you may have thoughts an feelings on something like this, too, if you're willing to take the time to evaluate and share. (no worries if not!)

Also, I'm not sure I love no-reaching-inside as a name, but I don't immediately have a better one in mind. I'd probably go toward something positive, i.e. import/shallow or something. But that is a more half-baked thought. I think it is important the name be as clear as possible, though, as it will live as long as the rule does.

@jfmengels
Copy link
Collaborator

Thanks @spalger!

Normally I'd like to see an issue with a proposal first

Same here, but this sounds interesting anyway :)

Not sure about the rule name either. I like the idea of shallow, but starting with no- is more ESLint like.

I do think that ../foo/index should be allowed though.

Not very constructive, but gotta go sorry ^^'

@spalger
Copy link
Contributor Author

spalger commented Aug 12, 2016

Normally I'd like to see an issue with a proposal first

Sorry :) Happy to see it go in, but not married to the idea if y'all would prefer a different direction entirely.

I'd probably go toward something positive, i.e. import/shallow or something.

I like the idea, but one thing I don't want to imply is that the rule is missing a depth parameter, which is a feeling I get with the name "shallow". In my opinion the rule is sort of useless if you can reach into a module at all (with the exception being non-module directories).

Adding minimatch is a non-trivial dependency

I would love to replace minimatch with another solution. Especially if it allow removing the path-separator-normalization dance. The initial implementation followed the lead of no-restricted-paths but it's dependency on process.cwd() meant it didn't work with linter-eslint in atom.

@benmosher
Copy link
Member

Could you refactor to use glob instead of minimatch? Looks like that's ESLint's globber of choice, I'd like to be consistent with them (and then, ideally, the dependency can be shared).

@spalger
Copy link
Contributor Author

spalger commented Aug 22, 2016

glob is about finding files on the file system that match the pattern, not about checking if a known path (the resolved path of an import statement) matches a pattern. minimatch is the matching library used by glob though, so the user experience should be exactly the same.

If you think it's necessary I can probably extract the minimatch pattern used by glob, but I think it would make more sense to depend on minimatch directly.

@benmosher
Copy link
Member

Ah, got it. As you can tell, I'm not very familiar with either. Just reminded me of an old ESLint issue about gitignore vs eslintignore. That makes sense, now.


## Rule Details

This rule has one option, `allow` which is an array of minimatch patterns to identify directories whose children can be imported explicitly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make minimatch patterns a link that links to documentation about minimatch patterns?

@jfmengels
Copy link
Collaborator

This looks pretty good to me. I still think that ../foo/index and ../foo/index.js should be allowed though, as that is what is happening most of the time under the hood when you require ../foo, and is then only a coding style preference.

@spalger
Copy link
Contributor Author

spalger commented Aug 23, 2016

@jfmengels In my eyes, the spirit of the rule is that the consumer of a module shouldn't be concerned with the implementation of the module. This includes where the entry point is.

If the module is using an index.js one day, moved to a single file the next, and ended up with a directory containing package.json and a custom main entry, the consumer shouldn't need to know.

@spalger
Copy link
Contributor Author

spalger commented Aug 23, 2016

@jfmengels I updated the rule so that allow now whitelists files that can be imported with reaching. It will first check for an allow pattern that matches the import statement, then an allow pattern that matches the resolved file path, and if either succeeds then the import will pass.

Because of this you could use allow: ["*/index{.js,}"] to support importing ../foo/index and ../foo/index.js but not ../foo/bar/index.js.

return !!find(allowRegexps, re => re.test(importPath))
}

// minimatch patterns are expected to use / path seperators, like import
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: seperators --> separators 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spell that word wrong everywhere, all the time, I think I need to setup autohotkey just for it.

@jfmengels
Copy link
Collaborator

Some nitpicks/code style improvements from me, but looks good to me otherwise :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 97.779% when pulling d0ce3a8 on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

@jfmengels
Copy link
Collaborator

I get the reasoning of not wanting to duplicate the plugin name, but I find import/no-internal-modules confusing as it makes me think that you're not allowed to have internal modules.

(PS : Bikeshedding is hard :D)

@benmosher
Copy link
Member

@jfmengels I think that's a reasonable concern in general, but in this case it's not obvious to me what incorrect behavior that name would imply.

i.e. what would you expect not to be allowed, given no-internal-modules as a name, that is allowed? or vice versa?

This rule is pretty restrictive in that way. Seems like a fair characterization.

@jfmengels
Copy link
Collaborator

what would you expect not to be allowed, given no-internal-modules as a name, that is allowed

Well, I wouldn't expect much to be allowed I guess if this rule forbid internal modules altogether :p
But in my mind, that's what the rule name says, even if it doesn't make much sense, and it's a bit hard for me to understand that it forbids importing an internal module, even though I know what it does.

You're right that naming and functionality-wise it's very close to no-nodejs-modules, but that is less confusing because I can't create core nodejs modules (now that I say it, I hope that it's clear that core modules are meant).

Let's put it this way: When I read the rule name, I read no internal modules, not import no internal modules (I don't read the plugin name when I read the rule). If you think that most people read it in the second way, then the name is fine by me.

@benmosher
Copy link
Member

I guess I want to err on the side of being concise, because absolutely no internal modules ever would be a wild world in which to try to code (and lint!).

@jfmengels
Copy link
Collaborator

Ok, let's go with that then.
@spalger Just want to make sure of something. what happens if I want to import lodash/fp or lodash/fp/get for instance? Do I need to put it in the allowed imports, or is that allowed by default?

@spalger
Copy link
Contributor Author

spalger commented Sep 6, 2016

You would need to add lodash/** or lodash/* to the allow patterns

@coveralls
Copy link

coveralls commented Sep 11, 2016

Coverage Status

Coverage increased (+0.04%) to 97.837% when pulling 3d00542 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

@benmosher
Copy link
Member

Thanks for merging from master! Could you go ahead and rename it to no-internal-modules?

@spalger
Copy link
Contributor Author

spalger commented Sep 12, 2016

Whoops, got distracted previously I guess 😄 . Updated!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.837% when pulling 152dd37 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage increased (+0.04%) to 97.837% when pulling 152dd37 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

@spalger spalger changed the title Implement new rule: no-reaching-inside Implement new rule: no-internal-modules Sep 12, 2016
@@ -0,0 +1,2 @@
require('babel-register')
require('./tests/src/rules/no-internal-modules.js')
Copy link
Member

@benmosher benmosher Sep 12, 2016

Choose a reason for hiding this comment

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

what is this file? looks like an impromptu describe.only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops :)

@jfmengels
Copy link
Collaborator

Apart from @benmosher's comment, this looks good to me. Thanks a lot @spalger! 😄

@jfmengels jfmengels mentioned this pull request Sep 12, 2016
4 tasks
@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage increased (+0.04%) to 97.837% when pulling b158a32 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

@benmosher benmosher added this to the 1.16.0 milestone Sep 13, 2016
Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

LGTM
(sorry, just wanted to use the new GitHub approval system :p)

@benmosher
Copy link
Member

lol @jfmengels, me too. @spalger: looks great! thanks for working with us!

@benmosher benmosher merged commit c57beae into import-js:master Sep 15, 2016
benmosher added a commit that referenced this pull request Sep 15, 2016
@spalger
Copy link
Contributor Author

spalger commented Sep 15, 2016

🎉 🎉 🎉 🎉 🎉

@jfmengels
Copy link
Collaborator

Hey, once again: Thanks a lot @spalger, this is awesome!

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

Successfully merging this pull request may close these issues.

4 participants