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] ExportMap / flat config: include languageOptions in context #3052

Merged

Conversation

michaelfaith
Copy link
Contributor

This change fixes a bug with flat config support. There is a function called childContext that's used by the ExportBuilder to "cleanse" the context object. This function wasn't including the new languageOptions object, which contains the parser. So by the time this cleansed context made it to the parse function, languageOptions wasn't there anymore.

Since parserPath was still being included in non-flat config scenarios, the parse function made it through ok and used parserPath. However, once you shift to flat config, parserPath is no longer defined, and the actual parser object needs to be there.

Fixes #3051

This change fixes a bug with flat config support.
There is a function called `childContext` that's used by the ExportBuilder to "cleanse" the context object.
This function wasn't including the new `languageOptions` object, which contains the parser.
So by the time this cleansed context made it to the parse function, `languageOptions` wasn't there anymore.

Since `parserPath` was still being included in non-flat config scenarios, the parse function made it through ok and used `parserPath`.
However, once you shift to flat config, `parserPath` is no longer defined, and the actual parser object needs to be there.

Fixes import-js#3051
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.29%. Comparing base (18787d3) to head (6ba9c0d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3052      +/-   ##
==========================================
- Coverage   95.13%   90.29%   -4.84%     
==========================================
  Files          81       81              
  Lines        3411     3411              
  Branches     1195     1195              
==========================================
- Hits         3245     3080     -165     
- Misses        166      331     +165     

☔ 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! Any way we can test for this?

@michaelfaith
Copy link
Contributor Author

Thanks! Any way we can test for this?

For sure. I can do that. I'm having issues running the unit test suite locally (I think my version of node is too new for this version of nyc), so it may take a few updates to the PR to get things just right, using CI as my feedback loop.

@michaelfaith michaelfaith force-pushed the fix/include-lanugage-options-in-context branch from 53ae632 to 6ba9c0d Compare September 5, 2024 17:17
@michaelfaith
Copy link
Contributor Author

Added a couple of tests for childContext (and I was able to get the tests running locally by updating the version of nyc to the latest, as well as making a few other tweaks to the setup).

@rarenatoe

This comment was marked as off-topic.

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! It'd be really nice to have an actual eslint 9 test that matches the flow that exposed this issue, but this is sufficient for now :-)

@ljharb ljharb force-pushed the fix/include-lanugage-options-in-context branch from 6ba9c0d to 186f248 Compare September 5, 2024 18:18
@ljharb ljharb changed the title fix(flatConfig): include languageOptions in context [Fix] ExportMap / flat config: include languageOptions in context Sep 5, 2024
@ljharb ljharb merged commit 186f248 into import-js:main Sep 5, 2024
308 of 309 checks passed
@michaelfaith
Copy link
Contributor Author

Thanks! It'd be really nice to have an actual eslint 9 test that matches the flow that exposed this issue, but this is sufficient for now :-)

Totally agree, and those'll be in @G-Rath's PR. Their v9 tests are failing right now because they need this change. So, once they bring this into their branch, those tests will be the v9 integration tests that cover this scenario.

@G-Rath
Copy link
Contributor

G-Rath commented Sep 5, 2024

(fwiw I did a poor cherry-pick before this landed and pushed that up and at least some of the tests were improved, and now locally I've rebased and am just sorting out the handling of env before I push again and start digging into the whole suite again etc etc 🙂)

@ljharb
Copy link
Member

ljharb commented Sep 5, 2024

I'm waiting for CI, and then I'll be publishing eslint-module-utils, and then updating main to require it and use the new helpers, after which you'll need to rebase, so feel free to wait :-)

renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 4, 2024
| datasource | package              | from   | to     |
| ---------- | -------------------- | ------ | ------ |
| npm        | eslint-plugin-import | 2.30.0 | 2.31.0 |


## [v2.31.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2310---2024-10-03)

##### Added

-   support eslint v9 (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)] \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`order`]: allow validating named imports (\[[#3043](import-js/eslint-plugin-import#3043)], thanks \[[@manuth](https://github.com/manuth)])
-   \[`extensions`]: add the `checkTypeImports` option (\[[#2817](import-js/eslint-plugin-import#2817)], thanks \[[@phryneas](https://github.com/phryneas)])

##### Fixed

-   `ExportMap` / flat config: include `languageOptions` in context (\[[#3052](import-js/eslint-plugin-import#3052)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export (\[[#3032](import-js/eslint-plugin-import#3032)], thanks \[[@akwodkiewicz](https://github.com/akwodkiewicz)])
-   \[`export`]: False positive for exported overloaded functions in TS (\[[#3065](import-js/eslint-plugin-import#3065)], thanks \[[@liuxingbaoyu](https://github.com/liuxingbaoyu)])
-   `exportMap`: export map cache is tainted by unreliable parse results (\[[#3062](import-js/eslint-plugin-import#3062)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   `exportMap`: improve cacheKey when using flat config (\[[#3072](import-js/eslint-plugin-import#3072)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   adjust "is source type module" checks for flat config (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)])

##### Changed

-   \[Docs] \[`no-relative-packages`]: fix typo (\[[#3066](import-js/eslint-plugin-import#3066)], thanks \[[@joshuaobrien](https://github.com/joshuaobrien)])
-   \[Performance] \[`no-cycle`]: dont scc for each linted file (\[[#3068](import-js/eslint-plugin-import#3068)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Docs] \[`no-cycle`]: add `disableScc` to docs (\[[#3070](import-js/eslint-plugin-import#3070)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Tests] use re-exported `RuleTester` (\[[#3071](import-js/eslint-plugin-import#3071)], thanks \[[@G-Rath](https://github.com/G-Rath)])
-   \[Docs] \[`no-restricted-paths`]: fix grammar (\[[#3073](import-js/eslint-plugin-import#3073)], thanks \[[@unbeauvoyage](https://github.com/unbeauvoyage)])
-   \[Tests] \[`no-default-export`], \[`no-named-export`]:  add test case (thanks \[[@G-Rath](https://github.com/G-Rath)])
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 4, 2024
| datasource | package              | from   | to     |
| ---------- | -------------------- | ------ | ------ |
| npm        | eslint-plugin-import | 2.30.0 | 2.31.0 |


## [v2.31.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2310---2024-10-03)

##### Added

-   support eslint v9 (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)] \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`order`]: allow validating named imports (\[[#3043](import-js/eslint-plugin-import#3043)], thanks \[[@manuth](https://github.com/manuth)])
-   \[`extensions`]: add the `checkTypeImports` option (\[[#2817](import-js/eslint-plugin-import#2817)], thanks \[[@phryneas](https://github.com/phryneas)])

##### Fixed

-   `ExportMap` / flat config: include `languageOptions` in context (\[[#3052](import-js/eslint-plugin-import#3052)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export (\[[#3032](import-js/eslint-plugin-import#3032)], thanks \[[@akwodkiewicz](https://github.com/akwodkiewicz)])
-   \[`export`]: False positive for exported overloaded functions in TS (\[[#3065](import-js/eslint-plugin-import#3065)], thanks \[[@liuxingbaoyu](https://github.com/liuxingbaoyu)])
-   `exportMap`: export map cache is tainted by unreliable parse results (\[[#3062](import-js/eslint-plugin-import#3062)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   `exportMap`: improve cacheKey when using flat config (\[[#3072](import-js/eslint-plugin-import#3072)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   adjust "is source type module" checks for flat config (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)])

##### Changed

-   \[Docs] \[`no-relative-packages`]: fix typo (\[[#3066](import-js/eslint-plugin-import#3066)], thanks \[[@joshuaobrien](https://github.com/joshuaobrien)])
-   \[Performance] \[`no-cycle`]: dont scc for each linted file (\[[#3068](import-js/eslint-plugin-import#3068)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Docs] \[`no-cycle`]: add `disableScc` to docs (\[[#3070](import-js/eslint-plugin-import#3070)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Tests] use re-exported `RuleTester` (\[[#3071](import-js/eslint-plugin-import#3071)], thanks \[[@G-Rath](https://github.com/G-Rath)])
-   \[Docs] \[`no-restricted-paths`]: fix grammar (\[[#3073](import-js/eslint-plugin-import#3073)], thanks \[[@unbeauvoyage](https://github.com/unbeauvoyage)])
-   \[Tests] \[`no-default-export`], \[`no-named-export`]:  add test case (thanks \[[@G-Rath](https://github.com/G-Rath)])
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.

Missing documentation on how to setup eslint-plugin-import with flat confiuration and Typescript
5 participants