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 tsconfig references #4689

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sushruth
Copy link

@sushruth sushruth commented Feb 6, 2025

Summary

Fixes #1648 -

  1. By actively checking if at least some files in the parsed tsconfig actually pass isTestFile() check.
  2. If project references are found in the parsed tsconfig, we iterate over each reference until we find a config which has at least one file that passes isTestFile() check.
  3. If we do not find any within references, we default to previous behavior.

Test plan

TODO

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@sushruth sushruth force-pushed the sushruth/feat-support-tsconfig-references branch 2 times, most recently from 3644197 to c2eee48 Compare February 6, 2025 08:27
@ahnpnl ahnpnl marked this pull request as ready for review February 6, 2025 09:15
@ahnpnl ahnpnl requested a review from kulshekhar as a code owner February 6, 2025 09:15
@ahnpnl ahnpnl marked this pull request as draft February 6, 2025 09:15
@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 6, 2025

Would you pls add some tests for this change?

@sushruth
Copy link
Author

sushruth commented Feb 6, 2025

@ahnpnl Absolutely. That is the reason I have left it in draft stage. Thanks.

@sushruth sushruth force-pushed the sushruth/feat-support-tsconfig-references branch from c2eee48 to daec5b7 Compare February 11, 2025 07:04
@sushruth sushruth marked this pull request as ready for review February 11, 2025 11:05
@sushruth
Copy link
Author

@ahnpnl I added an e2e test case covering the tsconfig project references scenario. Please check it out when you can. Thanks!

@@ -218,6 +216,9 @@ export class ConfigSet {
this._matchTestFilePath = globsToMatcher(
this._matchablePatterns.filter((pattern: string | RegExp) => typeof pattern === 'string') as string[],
)

this._setupConfigSet(options)
Copy link
Author

Choose a reason for hiding this comment

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

_setupConfigSet now depends on matchablePatterns to exist when its called, because isTestFile is used in finding the appropriate tsconfig reference project to use for ts compile/transpile process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we only check for test files, would it be like exceptional case when processing a random file? Should we also treat any files given through process method in transformer class similarly like this implementation?

Copy link
Author

Choose a reason for hiding this comment

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

When processing a random file, it defaults to the current behavior - finds a relevant tsconfig.json to pick, as it currently does.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it mean the handling with project reference config only applies to test files as you mentioned the file name needs to pass the check isTestFile?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the rationale is - if there is a specific tsconfig project reference that addresses jest files alone, we should use that tsconfig file. Otherwise we default to current behavior of using the root tsconfig.json file we found.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this can break any existing behaviors? In theory, it should fix instead of breaking.

In reality, this actually changes the existing behavior into a different behavior. Do you think that is a breaking change since we changed the behavior? I still suspect there might be edge cases where this change can cause a breaking change.

@@ -790,7 +790,6 @@ describe('_resolveTsConfig', () => {
expect(conf.options.configFilePath).toBeUndefined()
expect(findConfig).not.toHaveBeenCalled()
expect(readConfig.mock.calls[0][0]).toBe('/foo/tsconfig.bar.json')
expect(parseConfig).not.toHaveBeenCalled()
Copy link
Author

Choose a reason for hiding this comment

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

this will now be called to find out if we there are project references in the tsconfig to check.

@@ -918,7 +917,6 @@ describe('_resolveTsConfig', () => {

const conf = cs.parsedTsConfig
expect(conf.options.path).toBe(tscfgPathStub)
expect(findConfig).not.toHaveBeenCalled()
Copy link
Author

Choose a reason for hiding this comment

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

this will now be called to find out if we there are project references in the tsconfig to check.

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.

Tests fail when using TypeScript project references
2 participants