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

Add a quick note on how unbound imports and --fix #2640

Merged
merged 1 commit into from
Mar 22, 2024
Merged

Add a quick note on how unbound imports and --fix #2640

merged 1 commit into from
Mar 22, 2024

Conversation

mi-na-bot
Copy link
Contributor

Having unbound imports mixed among the bound ones causes unexpected and incorrect seeming results. I spent several hours trying to fix this problem only to find it was well known!

Since this is only a change to one markdown file, I assume no other tests are required. Hopefully this is helpful.

Having unbound imports mixed among the bound ones causes unexpected and incorrect seeming results. I spent several hours trying to fix this problem only to find it was well known!
@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.27%. Comparing base (c77c1a6) to head (6564860).

❗ Current head 6564860 differs from pull request most recent head aa17e43. Consider uploading reports for the commit aa17e43 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2640      +/-   ##
==========================================
+ Coverage   95.14%   95.27%   +0.13%     
==========================================
  Files          68       68              
  Lines        2967     2966       -1     
  Branches     1044     1008      -36     
==========================================
+ Hits         2823     2826       +3     
+ Misses        144      140       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This sounds fine, but in every eslint rule, you can never rely on autofix covering everything - you always have to fix some things manually.

docs/rules/order.md Outdated Show resolved Hide resolved
@ljharb ljharb added the docs label Jan 2, 2023
@mi-na-bot
Copy link
Contributor Author

@ljharb This doc change is not intended to speak negatively about the implementation of --fix, which is IMO totally correct. If it reads that way perhaps there is better wording? I am coming at this from the perspective of a new plugin user, and how the fix settled was somewhat unexpected around some css imports. I kind of thought something was broken, and with a hint from the manual I would have been in business much faster!

@ljharb
Copy link
Member

ljharb commented Jan 2, 2023

@MinervaBot right - that's why it's fine :-) but perhaps eslint's own documentation would have been a better place to set up proper expectations about autofixing?

@mi-na-bot
Copy link
Contributor Author

@MinervaBot right - that's why it's fine :-) but perhaps eslint's own documentation would have been a better place to set up proper expectations about autofixing?

Like "it doesn't always work"? Wow, not very helpful actually. How/why it doesn't work is 100% on these docs to explain (or not?)

@ljharb
Copy link
Member

ljharb commented Mar 22, 2024

@MinervaBot yes, because that's potentially the case in every eslint rule, including the ones in core.

@ljharb ljharb merged commit f3e505b into import-js:main Mar 22, 2024
175 of 176 checks passed
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Sep 4, 2024
| datasource | package              | from   | to     |
| ---------- | -------------------- | ------ | ------ |
| npm        | eslint-plugin-import | 2.29.1 | 2.30.0 |


## [v2.30.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2300---2024-09-02)

##### Added

-   \[`dynamic-import-chunkname`]: add `allowEmpty` option to allow empty leading comments (\[[#2942](import-js/eslint-plugin-import#2942)], thanks \[[@JiangWeixian](https://github.com/JiangWeixian)])
-   \[`dynamic-import-chunkname`]: Allow empty chunk name when webpackMode: 'eager' is set; add suggestions to remove name in eager mode (\[[#3004](import-js/eslint-plugin-import#3004)], thanks \[[@amsardesai](https://github.com/amsardesai)])
-   \[`no-unused-modules`]: Add `ignoreUnusedTypeExports` option (\[[#3011](import-js/eslint-plugin-import#3011)], thanks \[[@silverwind](https://github.com/silverwind)])
-   add support for Flat Config (\[[#3018](import-js/eslint-plugin-import#3018)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])

##### Fixed

-   \[`no-extraneous-dependencies`]: allow wrong path (\[[#3012](import-js/eslint-plugin-import#3012)], thanks \[[@chabb](https://github.com/chabb)])
-   \[`no-cycle`]: use scc algorithm to optimize (\[[#2998](import-js/eslint-plugin-import#2998)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[`no-duplicates`]: Removing duplicates breaks in TypeScript (\[[#3033](import-js/eslint-plugin-import#3033)], thanks \[[@yesl-kim](https://github.com/yesl-kim)])
-   \[`newline-after-import`]: fix considerComments option when require (\[[#2952](import-js/eslint-plugin-import#2952)], thanks \[[@developer-bandi](https://github.com/developer-bandi)])
-   \[`order`]: do not compare first path segment for relative paths (\[[#2682](import-js/eslint-plugin-import#2682)]) (\[[#2885](import-js/eslint-plugin-import#2885)], thanks \[[@mihkeleidast](https://github.com/mihkeleidast)])

##### Changed

-   \[Docs] `no-extraneous-dependencies`: Make glob pattern description more explicit (\[[#2944](import-js/eslint-plugin-import#2944)], thanks \[[@mulztob](https://github.com/mulztob)])
-   \[`no-unused-modules`]: add console message to help debug \[[#2866](import-js/eslint-plugin-import#2866)]
-   \[Refactor] `ExportMap`: make procedures static instead of monkeypatching exportmap (\[[#2982](import-js/eslint-plugin-import#2982)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Refactor] `ExportMap`: separate ExportMap instance from its builder logic (\[[#2985](import-js/eslint-plugin-import#2985)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Docs] `order`: Add a quick note on how unbound imports and --fix (\[[#2640](import-js/eslint-plugin-import#2640)], thanks \[[@MinervaBot](https://github.com/minervabot)])
-   \[Tests] appveyor -> GHA (run tests on Windows in both pwsh and WSL + Ubuntu) (\[[#2987](import-js/eslint-plugin-import#2987)], thanks \[[@joeyguerra](https://github.com/joeyguerra)])
-   \[actions] migrate OSX tests to GHA (\[[ljharb#37](ljharb/eslint-plugin-import#37)], thanks \[[@aks-](https://github.com/aks-)])
-   \[Refactor] `exportMapBuilder`: avoid hoisting (\[[#2989](import-js/eslint-plugin-import#2989)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Refactor] `ExportMap`: extract "builder" logic to separate files (\[[#2991](import-js/eslint-plugin-import#2991)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Docs] \[`order`]: update the description of the `pathGroupsExcludedImportTypes` option (\[[#3036](import-js/eslint-plugin-import#3036)], thanks \[[@liby](https://github.com/liby)])
-   \[readme] Clarify how to install the plugin (\[[#2993](import-js/eslint-plugin-import#2993)], thanks \[[@jwbth](https://github.com/jwbth)])
SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this pull request Sep 4, 2024
SukkaW added a commit to un-ts/eslint-plugin-import-x that referenced this pull request Sep 4, 2024
* fix: backports import-js#3033

* ci: cancel-in-progress when pr is updated

backports import-js#2997

* fix: backports import-js#2952

* docs: improve `order` rule docs

Backports:
- import-js#2640
- import-js#3036
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants