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

Advanced configuration of no-private rule based on element types #198

Open
dantman opened this issue Feb 1, 2022 · 4 comments
Open

Advanced configuration of no-private rule based on element types #198

dantman opened this issue Feb 1, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@dantman
Copy link

dantman commented Feb 1, 2022

Describe the bug
If a file of one element imports a file that is of a different element which is is a parent/uncle/etc no-private will block it.

To Reproduce
I've created a demo repo on StackBlitz where you can npm run lint.

https://stackblitz.com/edit/node-jtplrw?file=.eslintrc.js

Expected behavior
In the example, the unit-test file should be able to import the ui file from its parent. Instead of getting an error that the file belonging to its parent is private.

Logs

/home/projects/node-jtplrw/src/modules/module-a/ui/MyButton.spec.tsx
  1:26  error  Dependency is private of element of type 'modules' with module 'module-a'  boundaries/no-private

Additional context
This can happen easily if you have a bunch of different element types. Having a variety of element types shouldn't be unexpected, as boundaries/externals exists and is a great way of defining rules like "only tests can import test libraries", "only UI components in our design library can import components from external UI libraries, other components must be made up of our UI components", or "only *.web.js files can import libraries that assume react-dom is present, for react-native-web users".

I understand that if you have something like a "server" element with a "module" element inside of it. Then the idea of no-private not crossing element boundaries makes sense. But in the context of lighter element types this doesn't work out. Maybe the solution is for no-private to have an option for the change of behaviour or an option with an element matcher for what types of element boundaries should allow crossing / not make an element private.

@javierbrea
Copy link
Owner

Hi @dantman,

I agree, given the great amount of different possible scenarios, it is a good idea to allow configuring this rule based on element types. But, maybe allowing to import any private element may be dangerous, so I think it would be also useful to define a new option allowAncestors, which would allow to import only parents, grandparents, etc. (maybe a number to indicate the level of the ancestor could be also useful). I will add a draft of the new configuration to this issue before implementing it in order to get some feedback.

For the moment, until the issue is solved, maybe you can use the eslint overrides option to override the configuration of this rule only for your spec files:

{
  overrides: [
      {
        files: ["*.spec.tsx"],
        rules: {
          "boundaries/no-private": [0],
        }
      },
  ]
}

Note: I will rename this issue to "Advanced configuration of no-private rule based on element types".

Thank you very much for your feedback 😃

@javierbrea javierbrea added this to To do in Open source issues via automation Feb 1, 2022
@javierbrea javierbrea added the enhancement New feature or request label Feb 1, 2022
@javierbrea javierbrea changed the title [no-private] A file is not allow to import a file from its parent if that parent is of another type Advanced configuration of no-private rule based on element types Feb 1, 2022
@dantman
Copy link
Author

dantman commented Feb 1, 2022

But, maybe allowing to import any private element may be dangerous, so I think it would be also useful to define a new option allowAncestors, which would allow to import only parents, grandparents, etc. (maybe a number to indicate the level of the ancestor could be also useful).

I actually thought this was already the case when you had allowUncles enabled. As the docs state:

Private elements can import another private element if it is a direct child of a common ancestor, and the allowUncles option is enabled.

So my biggest confusion was discovering this rule only applies when both private elements are of the same type.

@dantman
Copy link
Author

dantman commented Feb 1, 2022

For better reference, my actual use case for no-private.

I have feature elements at src/{feature}, like "api", "theme", "layout", etc. And route elements at src/routes/{route}. Some of these features have an index.ts and discourage direct imports but others allow direct imports. So locking down imports with boundaries/entry-point doesn't necessarily make sense.

However various features, routes, and other elements do have an internal/ folder or internal.ts file for internal implementation stuff. e.g. a React context instance that is used by multiple files in the module but externally should only be accessed through the hooks/components exported by the feature. i.e. I don't want src/somefeature/foo.ts to import src/api/internal.ts.

no-private would make this work as internal.ts would be an internal element inside the feature (api) element and foo.ts is in a separate feature (somefeature). Unfortunately I also have other patterns codified as elements (utilities in utils/, UI components in ui/, custom form fields in fields/, unit tests in *.spec.ts(x), Storybook defs in *.stories.ts(x)) which define boundaries that have certain behaviours, but don't necessarily themselves mean "do not touch outside my direct parent". Some like ui/ make sense to be accessed by any descendent. While others like unit tests don't make sense to import (have an element-types rule forbidding that of course) but do inherently have to import a module from their direct parent.

Maybe this all could be better expressed with element-type, but it would need to be a lot more advanced. As currently it seems to choke as soon as there happens to be a minor element inside the major element you're trying to define rules for. I'll have to make an issue for that later.

@javierbrea
Copy link
Owner

javierbrea commented Feb 1, 2022

The allowUncles option is intended to allow importing only "brothers of a parent", or "brothers of a grandparent", etc. But not direct parents or grandparents. Why? Well, it tries to cover some kind of scenarios in which you have an structure like the next one:

└─── A
    ├── B
    │   └── C
    ├── D
    └── E

Then, you want to allow importing D and E from C. So you set allowUncles as true and the rule allows it. But it does not allow to importing B or A from C. Mainly, because in theory it would produce a circular dependency, given that if a package is private of another one, it should import it.

In your case, as the private element contains tests, I suppose that the parent element is not importing it, so it would have sense to allow a child to import its parents. That is the reason of my proposal for the allowAncestors option.

The type of elements shouldn't be affecting to this rule, and, in case it is, of course it should be fixed. Could you please provide an example of code and your configuration in order to reproduce it?

If it is not a bug, and it is related only to some misunderstanding about the rules, please provide a schematic example of your structure, and a list of the boundaries that you want to implement on it, so maybe I can help you with the configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

No branches or pull requests

2 participants