javierbrea / eslint-plugin-boundaries

Eslint plugin checking architecture boundaries between elements
MIT License
473 stars 9 forks source link

Running from a different working directory #296

Closed wadamek65 closed 10 months ago

wadamek65 commented 11 months ago

Describe the bug Not sure if this is actually an issue or something undocumented or even if there is anything wrong with this plugin or other parts of the solution. Would appreciate some pointers on what to look at/what to configure.

The problem is, when running the eslint script, from a directory different than the root of the linted project, the plugin will not give the correct output. The same configuration/rules will report errors when run in the root of the project, and will not report anything when run outside of the project.

To Reproduce Run the lint command from parent/child directory (different working directory than the root of the linted project)

npm exec -- eslint epb-ts-example-main/

or

npm exec -- eslint ../

Expected behavior The correct output is produced.

Logs If applicable, add logs to help explain your problem.

Operating system, Node.js an npm versions, and eslint version (please complete the following information):

Additional context While at first I thought this was an issue with our monorepo configuration, I seem to have the exact same problem with the example repository: https://github.com/javierbrea/epb-ts-example

If I for example, go into the src/ directory, and run the npm exec -- eslint ../ command from there, the output is no longer correct and the error is not reported.

javierbrea commented 10 months ago

Hi @wadamek65 , first of all, thanks for reporting the problem πŸ˜ƒ .

This happens because the plugin needs to know from where to resolve paths in order to make them match with the path matchers in the element-types setting. So, it assumes that the root of the project is the process.cwd() and it removes it from the file path and import paths before executing the match patterns. It is true that it may be a wrong assumption if you execute the linter from another path, as you have explained 😞 . But, after some research, I have not found a way to determine the path to be considered the "project root". Eslint does not provide to the plugins more info at this respect than the process.cwd itself, and it may have sense, because, which other information could we use to determine the "project root"? I have thought about:

So, the only solution I can think of is to add a setting to the plugin in order to allow to define the root path from where micromatch patterns must be resolved. This may solve the problem, but it wouldn't be compatible with defining configuration using .json format, for example, because the root path should have to be provided programmatically in most cases (by using __dirname, for example). As an alternative for this case, maybe we should also allow to define the root path by using an environment variable (this is not the ideal scenario, because I don't like too much to use configuration methods out of the eslint configuration mechanisms, but maybe we'll have to make an exception in this case).

In short, to solve it we could add a boundaries/root-path setting that could be used like:

// Note: .eslintrc.js file in the root path of the project
{
  settings: {
    "boundaries/root-path": __dirname,
  }
}

And, in case the configuration is defined using JSON, it could be defined also by using an environment variable. In case the provided value is a relative path, it would be resolved from the current process.cwd():

ESLINT_PLUGIN_BOUNDARIES_ROOT_PATH=../ npm exec -- eslint ../

Please let me know what do you think about this proposal.

wadamek65 commented 10 months ago

Hey @javierbrea , thanks a lot for responding and looking into it!

Since this seems like a rather niche use case, I too believe the best approach would be to keep things fully configurable via a custom rule, just in case there are more nasty edge cases where some kind of smarter file path deduction would fail.

As for your concerns regarding the JSON/JS configs, I'm not sure if there is a need to add special handling with an env variable. Since ESLint is most of the time run under preset conditions, the users should be able to provide string paths to whatever directory they want to run from. You wouldn't be able to pass a __dirname to an environment variable unless you are willing to inline a script in the command itself to resolve the path to a static string before the runtime. So, in the end, it seems like it will be exactly the same as just specifying a static string in the JSON config. On the other hand, it may be convenient for others to configure it like that, but it could possibly add feature bloat to the plugin and one more thing for you to support and maintain.

javierbrea commented 10 months ago

Hi again @wadamek65 , sorry, but I don't understand how would be the plugin able to resolve the directory to work from by simply using a string containing a relative path, because If you change the working directory, then the absolute path would be calculated from there, and it may be wrong. As far as I can see, it would need an absolute path, so it could be executed from any working directory, as it would never use it to resolve anything. That's why I suggested to use "resolve" to provide the setting in a JavaScript file, to make it absolute always, so it is not affected by the working directory.

About the env variable, it's just a workaround to be able to provide another "working directory" to the plugin when you are not able to provide an absolute path. In this case, you'll be able to provide a relative path, that would be resolved with the current cwd. This way, it would be your responsibility to make the plugin point to the project root. For example, if you are running the command one level below the project root, you should provide "ESLINT_PLUGIN_BOUNDARIES_ROOT_PATH=../", if you are two paths below the project root, you should provide "../../", etc. Or you could even provide an absolute path, of course. So, there is no script implied, it is just a mechanism to allow the user to define the project root when they can't define an absolute path in the config.

I probably didn't explain it well in my previous comment, I hope now I have done it better. πŸ˜ƒ

wadamek65 commented 10 months ago

Ah, I see. Thanks for the explanation, I understand it now.

I assumed that the behavior that you described for the env variable would be the same for the new rule, but that's not the case. In our case, it would be enough to be able to reconfigure the path slightly, like with the env variable behavior you described. I think that most people want to run ESLint from the same directories always. It's just that in some cases, especially monorepos, it's wrong to assume it will be run from the same root directory as the source code.

Just to make it clear, I think what you are proposing sounds all good and would definitely solve our problem πŸ‘ Just wanted to shine some light on our specific use case.

javierbrea commented 10 months ago

Thanks for notifying me about the problem πŸ‘ . I didn't think in that use case, mainly because I use to use Nx as monorepo tool, and it always uses each project root as working directory when running a command on each project, no matter the repository path in which you are. But this is something to solve, because, as you said, it's wrong to assume that the lint command will be run from the same root directory always. So, I add it to the backlog and I hope to solve it soon πŸ˜ƒ

wadamek65 commented 10 months ago

@javierbrea Interesting, because we're also using nx and running the command nx run-many -t lint from the root of the repo and use the nx ESLint plugin within our packages, but we're not getting the correct output unless we explicitly go into package directory and run ESLint from there. Perhaps OS differences since the majority of us are working on Windows πŸ€” Perhaps the configuration needs to exist in the root as well with all the paths accounted for separately for each package? πŸ€”

Regardless, this will definitely help us out, so thanks a lot!

javierbrea commented 10 months ago

@wadamek65 , mmm... I don't think it is related to the OS, because we use different OS in the team, and it works properly in all cases. We create a different "lint" command in the package.json file of each monorepo project, each one with its own eslint config (that may be reused by exporting it from another project). This way, the affected Nx command lints only affected projects. With this configuration, we have no problems, as Nx uses the project root to run the command. But anyway, there are multiple ways of doing things, and the plugin should be able to work with any of them.