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: add languageOptions to ChildContext #85

Merged
merged 7 commits into from
Jun 30, 2024

Conversation

kosmotema
Copy link

Currently the plugin loses languageOptions when creating a child context (when using a flat config). This causes errors when plugin tries to use the default parser, i.e. languageOptions.parser

I've also created a reproduction for this bug

Copy link

changeset-bot bot commented May 26, 2024

🦋 Changeset detected

Latest commit: 4ece153

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-import-x Patch

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

Copy link

codesandbox-ci bot commented May 26, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@kosmotema
Copy link
Author

I extracted the logic of creating the context cache key into a separate function, and also made a function for obtaining a hash for options with its caching (since adding 2 more global variables for parser.meta doesn't seem so good)

@n0099
Copy link

n0099 commented Jun 6, 2024

This pr seems similar with import-js#2829 but with additional cache and both should fix Parse errors in imported module '': parserPath or languageOptions.parser is required!: import-js#2556 (comment)

n0099 added a commit to n0099/open-tbm that referenced this pull request Jun 6, 2024
* fix `Parse errors in imported module '': parserPath or languageOptions.parser is required!`:  un-ts/eslint-plugin-import-x#85 (comment)
@ eslint.config.js

* re-plug eslint: pzmosquito/eslint-import-resolver-vite#12 (comment) @ package.json
* fix some violations of eslint rules
@ fe
@kosmotema kosmotema requested a review from JounQin June 21, 2024 22:40
@JounQin JounQin requested a review from SukkaW June 24, 2024 13:32
src/utils/export-map.ts Outdated Show resolved Hide resolved
@kosmotema
Copy link
Author

Refactored a bit context hashing:

  • now all cached properties are declared when the object is created
  • also I decided to use dequal instead of JSON.stringify and store the actual values in the cache instead of their json

A little more on the second point:
I've noticed that settings and languageOptions (for flat config) often doesn't change when childContext is called again (i.e. they're referentially equal). So it seems that jsoning the values each time is often unnecessary and inefficient, and we can just use deep equal approach for comparing (which will handle the referentially equality case faster)

Copy link
Collaborator

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

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

@kosmotema I noticed that a few tests were not passed and CI failed. I am blocking the PR for now.

@kosmotema kosmotema force-pushed the fix/child-context-language-options branch from 75f7705 to 4ece153 Compare June 29, 2024 20:34
@kosmotema
Copy link
Author

Found and fixed the bug that causes this test to fail. For some reason I thought before that parserPath and languageOptions are exclusive (i.e. if there is languageOptions, parserPath will be nullish). It turned out that it is not quite so :(
Now to get a real parser, an algorithm similar to getParser is used

Also decided to switch from hashing approach to the versioning one. Since we already use deep equality to compare context options, it seems that hashing is not really necessary. So we can simply increment the version when some options change, and return that version as a string

@kosmotema kosmotema requested a review from SukkaW June 29, 2024 20:47
@JounQin JounQin removed their request for review June 30, 2024 04:07
@SukkaW
Copy link
Collaborator

SukkaW commented Jun 30, 2024

Since we already use deep equality to compare context options, it seems that hashing is not really necessary. So we can simply increment the version when some options change, and return that version as a string

This reminds me of a package https://www.npmjs.com/package/stable-hash that does exactly that, IMHO we can use this package instead of self implementing this!

@SukkaW SukkaW merged commit ded3e80 into un-ts:master Jun 30, 2024
15 checks passed
robtaylor pushed a commit to robtaylor/eslint-plugin-import-x that referenced this pull request Aug 8, 2024
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.

4 participants