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

Resolve other plugins as siblings to this plugin #342

Merged
merged 2 commits into from
May 25, 2024

Conversation

onlywei
Copy link
Contributor

@onlywei onlywei commented May 13, 2024

Why do I want this?
I have a monorepo with the current file structure:

apps/
-  app1/
  -  package.json
   .............
-  app127/
  -  package.json
packages/
-  package1/
  -  package.json
   ............
-  package453/
  -  package.json
repotools/
-  central-linter/
  -  package.json
.eslintrc.json
.eslintignore

I want to install eslint and all of its configs and plugins into the repotools/central-linter/node_modules. I do not want to install a separate copy of eslint and its plugins in each app/package. Also note that there is no package.json file at the root of the repo because I have lots of issues with npm workspaces.

I also want to keep the .eslintrc.json and .eslintignore files at the root of the monorepo for better IDE integration.

I am able to accomplish all of this except for this eslint-plugin-jsonc since this plugin keeps trying to load plugins relative to the root of the monorepo, rather than the resolvePluginsRelativeTo that I passed to my central linter.

This PR should modify the behavior of eslint-plugin-jsonc to always resolve plugins relative to itself.

P.S. I cannot upgrade to ESLint v9 yet because nx and eslint-plugin-react does not support ESLint v9 yet.

Copy link

changeset-bot bot commented May 13, 2024

🦋 Changeset detected

Latest commit: b6a1cb4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-jsonc Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@onlywei
Copy link
Contributor Author

onlywei commented May 22, 2024

@ota-meshi Whenever you get a chance, could you please take a look at this PR? Thank you!

@ota-meshi
Copy link
Owner

Hmm. I'm not sure if this PR change will work in anyone's environment.
Can you please explain that it's not just your environment that works?

@onlywei onlywei force-pushed the relative-plugins branch from d1728a9 to b6a1cb4 Compare May 24, 2024 18:50
@onlywei
Copy link
Contributor Author

onlywei commented May 24, 2024

Hmm. I'm not sure if this PR change will work in anyone's environment. Can you please explain that it's not just your environment that works?

@ota-meshi My thinking is this: two scenarios:

Scenario One: with --resolve-plugins-relative-to

In this scenario, the folder structure is as follows:

some/other/path/
  node_modules/
    eslint-plugin-prettier/
    eslint-plugin-jsonc/
      dist/
        utils/
          get-auto-jsonc-rules-config/
            index.ts
.eslintrc.cjs

Eslint is executed on the CLI using:

eslint --resolve-plugins-relative-to=some/other/path/ .

We want to resolve the eslint-plugin-prettier that is a sibling of eslint-plugin-jsonc. From dist/utils/get-auto-jsonc-rules-config/, we need to navigate up five directories in order to be at another-path/. Once we are there, require('eslint-plugin-prettier') should work and find the correct path.

Scenario One: WITHOUT --resolve-plugins-relative-to

In this scenario, which is how most people use eslint, the folder structure is as follows:

node_modules/
  eslint-plugin-prettier/
  eslint-plugin-jsonc/
    dist/
      utils/
        get-auto-jsonc-rules-config/
          index.ts
.eslintrc.cjs

Eslint is executed on the CLI using:

eslint .

From dist/utils/get-auto-jsonc-rules-config/, if we navigate up five directories, we will be at the root of this folder structure. The behavior here should be the exact same as if I had not submitted this PR at all.

Therefore, I believe this PR should work in both scenarios.

@ota-meshi
Copy link
Owner

Thank you for the explanation! I think that explanation makes sense!

Copy link
Owner

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your contribution!

@ota-meshi ota-meshi merged commit 8cc5b7f into ota-meshi:master May 25, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants