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

feat: support extra options argument for resolver #2519

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JounQin
Copy link
Collaborator

@JounQin JounQin commented Aug 7, 2022

close #2108

@JounQin JounQin requested a review from ljharb August 7, 2022 04:31
@JounQin JounQin force-pushed the feat/resolver_options branch 2 times, most recently from 6c678d2 to 761b799 Compare August 7, 2022 04:47
@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04 ⚠️

Comparison is base (683d3a5) 95.19% compared to head (877b4ca) 95.16%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2519      +/-   ##
==========================================
- Coverage   95.19%   95.16%   -0.04%     
==========================================
  Files          68       66       -2     
  Lines        2977     2831     -146     
  Branches     1028      951      -77     
==========================================
- Hits         2834     2694     -140     
+ Misses        143      137       -6     
Impacted Files Coverage Δ
src/ExportMap.js 100.00% <ø> (+10.52%) ⬆️
utils/resolve.js 88.13% <100.00%> (+0.84%) ⬆️

... and 59 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JounQin JounQin marked this pull request as draft August 8, 2022 00:55
@JounQin
Copy link
Collaborator Author

JounQin commented Aug 8, 2022

I just found I forget to add something like resolved ts compile options into the argument.

Done!

@JounQin JounQin changed the title feat: add context into resolver arguments feat: support extra options argument for resolver Aug 10, 2022
@JounQin JounQin marked this pull request as ready for review August 10, 2022 08:43
@JounQin JounQin force-pushed the feat/resolver_options branch 2 times, most recently from 80be205 to 7c7e472 Compare August 13, 2022 04:16
@JounQin
Copy link
Collaborator Author

JounQin commented Aug 13, 2022

Friendly ping @ljharb

@ljharb
Copy link
Member

ljharb commented Aug 13, 2022

I'm moving slowly on this one because of the potential for breakage.

Specifically, "old utils + old plugin", and "new utils + new plugin", obviously will work fine. However, I need to convince myself that "new utils + old plugin" and "old utils + new plugin" will continue to work. I'm likely going to wait until I've released all the unreleased changes before merging this.

@JounQin
Copy link
Collaborator Author

JounQin commented Aug 30, 2022

I should pass the flattened tsconfig instead of tsCompilerOptions for all edge cases.

src/ExportMap.js Outdated
@@ -24,7 +24,7 @@ let ts;
const log = debug('eslint-plugin-import:ExportMap');

const exportCache = new Map();
const tsConfigCache = new Map();
const tsconfigCache = new Map();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rename it as tsconfigCache because tsconfig is much common usage.

@MWhite-22
Copy link

Any chance this PR can get some love @ljharb?

This PR helps resolve the eslint monorepo issues regarding looking for a single root tsconig vs individual package level tsconfigs.

@ljharb
Copy link
Member

ljharb commented Sep 26, 2022

@MWhite-22 #2519 (comment)

@MWhite-22
Copy link

MWhite-22 commented Sep 26, 2022

I'm moving slowly on this one because of the potential for breakage.

[...] I'm likely going to wait until I've released all the unreleased changes before merging this.

When you say "All of the unreleased changes", I am just trying to get a rough approximation for what that looks like. Are we talking a few weeks or a few months?

@ljharb
Copy link
Member

ljharb commented Sep 26, 2022

I don't cut releases on any particular timeline. It could be hours, or weeks.

@nVitius
Copy link

nVitius commented Nov 29, 2022

@JounQin Looks like there's a very small merge conflict on your branch. Can you resolve so this PR can move into a mergeable state again?

@ljharb I just wanted to echo the sentiment that this would be great to have. Currently setting up a monorepo with TS and I would love to use this plugin.

Have you had more time to think about the inclusion of this PR?

@MWhite-22
Copy link

Wanted to check in and see if there might be any option to help contribute to getting this merged?

Would love to revive this feature as its a prerequisite to a few other packages.

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.

I've rebased this PR and split it up into separate commits - the final commit is the one that I'm concerned about:

  1. it has no tests - the tests are in the third commit, and relate to resolvers, but we need tests that relate to the ExportMap changes
  2. is this potentially a breaking change, if someone's using an older version of eslint-module-utils, with the ExportMap changes?

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.

[feature] make context.getCwd() available in resolver
4 participants