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

Fix other false-positives with node_modules #10

Merged
merged 2 commits into from
Jun 11, 2023

Conversation

mozisan
Copy link
Contributor

@mozisan mozisan commented Jun 7, 2023

問題

defaultImportability=packageの際、ライブラリに対するインポートをLintの対象外にするような差分を入れていましたが、Monorepo構成などでsymlinkを使ってインストールされるライブラリは「node_modules配下」という判定にならず、再びエラーが出てしまいました 😢

対応方針

現状では以下の箇所のコードで、require.resolve("foo")あるいはrequire.resolve("foo/package.json")を試し、ここで得られたパスが/node_modules/という文字列を含むかどうかという判定を行なっています。
https://github.com/uhyo/eslint-plugin-import-access/blob/master/src/rules/jsdoc.ts#L190-L205
ここでsymlinkが使われるとパスが/node_modules/を含まないという結果になり、結果としてLintの対象になってしまいました。

そこで、require.resolve("foo/package.json")へのパス解決が成功するかどうかを優先的に扱うことを考えたのですが、その場合に今度はimport sub from "foo/sub";のようなサブモジュールへのインポートに対する制御が面倒になってしまいます。

ちょっと色々やろうとは思ったのですが、すでにこういった判定処理が実装されたライブラリがないか探してみたところ、resolve-pkgenhanced-resolveというライブラリがあり、まさにこれだと思ってこのライブラリを使うように変えてみました。

その他の変更点

  • defaultImportability=package時のライブラリ関連のテストをsrc/__tests__/fixtures/project/src/default-export{3,4}に書いていましたが、これらはDefault exportに対する挙動をテストしたいものではなかったので、ライブラリ関連のテストをsrc/__tests__/fixtures/project/src/library/**以下に再配置しました。
  • ライブラリへのインポートをテストする際、テスト用のライブラリをsrc/__tests__/fixtures/packages/*に用意していましたが、「symlinkでインストールされるテスト用ライブラリ」も用意したかったため、以下のようなディレクトリ構造に変更しました。
    • src/__tests__/fixtures/packages/third-party/*
      • symlinkでインストールされない通常のライブラリを想定したものを配置する
      • .npmrcinstall-links=trueと記述した上で、npm i -D src/__tests__/fixtures/packages/third-party/fooを実行するとテストで利用できるようになる
    • src/__tests__/fixtures/packages/workspaces/*
      • Monorepoのworkspacesとして定義されるライブラリのような、symlinkでイントールされることを想定したものを配置する
      • package.jsonworkspaces項目を追加し、npm i -D <package-name>を実行するとテストで利用できるようになる(npmのworkspace機能はinstall-links=trueに影響されずにsymlinkでインストールされる)

@mozisan mozisan marked this pull request as ready for review June 7, 2023 17:37
@mozisan
Copy link
Contributor Author

mozisan commented Jun 7, 2023

あ、いやまだ正常に検知できないケースが残っていました・・・:sob:
package.jsonexportsフィールドが定義されていて、かつpackage.jsonexportsフィールドで公開されていない場合、resolve-pkgはそのライブラリをライブラリだと判定できないようです。
そしてそれをうまいこと解消する良さそうな方法はまだなさそうという・・・
sindresorhus/resolve-pkg#9 (comment)

@mozisan mozisan force-pushed the fix/node-modules-false-positive branch from 56faf58 to 456bcdf Compare June 8, 2023 19:59
@mozisan
Copy link
Contributor Author

mozisan commented Jun 8, 2023

eslint-plugin-importを参考に見ていたところ、この内部で使われているenhanced-resolveがライブラリかどうかの判定に良さそうだったのでそちらに切り替えてみました。
業務で使っているプロジェクトもこれでFalse positiveを取り除けました!!

Copy link
Owner

@uhyo uhyo left a comment

Choose a reason for hiding this comment

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

遅くなりすみません。ありがとうございます 🥰

@uhyo uhyo merged commit a3c7d80 into uhyo:master Jun 11, 2023
@mozisan mozisan deleted the fix/node-modules-false-positive branch June 11, 2023 11:59
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