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

Refactor export map builder #2991

Closed
wants to merge 0 commits into from

Conversation

soryy708
Copy link
Contributor

exportMapBuilder.js contains a huge god-object with 651 lines of code, with tightly coupled procedures that are hard to follow.

Lets extract procedures and classes, and put them in well named files.
This separation will also help with encapsulation and decoupling.

It's not perfect yet, but a reduction from 651 lines to 207 lines is a 68% decrease.

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 78.63501% with 72 lines in your changes are missing coverage. Please review.

Project coverage is 82.45%. Comparing base (2de78c1) to head (c986ba3).

Files Patch % Lines
src/exportMap/captureDependency.js 46.15% 14 Missing ⚠️
src/exportMap/builder.js 86.51% 12 Missing ⚠️
src/exportMap/typescript.js 40.00% 12 Missing ⚠️
src/exportMap/visitor.js 87.05% 11 Missing ⚠️
src/exportMap/childContext.js 42.85% 8 Missing ⚠️
src/exportMap/patternCapture.js 75.00% 6 Missing ⚠️
src/exportMap/doc.js 84.37% 5 Missing ⚠️
src/exportMap/specifier.js 91.30% 2 Missing ⚠️
src/exportMap/namespace.js 94.73% 1 Missing ⚠️
src/exportMap/remotePath.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2991      +/-   ##
==========================================
+ Coverage   81.81%   82.45%   +0.63%     
==========================================
  Files          77       86       +9     
  Lines        3795     3813      +18     
  Branches     1257     1256       -1     
==========================================
+ Hits         3105     3144      +39     
+ Misses        690      669      -21     

☔ 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.

Thanks, this is great!

It seems like this refactor is exposing a lot of previously uncovered lines. It's fine to land this without covering them, but it would be even better if this PR (or another one, landed first) could include additional tests so we can ensure the uncovered lines are all covered

src/exportMap/patternCapture.js Outdated Show resolved Hide resolved
@@ -0,0 +1,60 @@
export function captureDependency(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function captureDependency(
export default function captureDependency(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would captureDependency be the default and not, say, captureDependencyWithSpecifiers that is exported from the same file?

Copy link
Member

Choose a reason for hiding this comment

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

because the filename is "captureDependency". a default export is what a module is, a named export is something a module has, and most modules should be something.

src/exportMap/childContext.js Outdated Show resolved Hide resolved
src/exportMap/namespace.js Outdated Show resolved Hide resolved
src/exportMap/specifier.js Outdated Show resolved Hide resolved
src/exportMap/visitor.js Outdated Show resolved Hide resolved
@soryy708
Copy link
Contributor Author

soryy708 commented Apr 1, 2024

this refactor is exposing a lot of previously uncovered lines. It's fine to land this without covering them

I reviewed the lines that aren't covered.
Lets land this without covering them for now.
I can't think of good & quick tests to write for them at this time.

@soryy708
Copy link
Contributor Author

soryy708 commented Apr 5, 2024

Has been merged via

@soryy708 soryy708 closed this Apr 5, 2024
@ljharb ljharb reopened this Apr 5, 2024
@ljharb ljharb closed this Apr 5, 2024
@ljharb ljharb force-pushed the refactor-export-map-builder branch from 6d8a0b8 to 8587c85 Compare April 5, 2024 14:22
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)])
@soryy708 soryy708 deleted the refactor-export-map-builder branch September 23, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants