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

Support multiple tsconfig files in expect #973

Merged
merged 16 commits into from
Mar 8, 2024

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Mar 5, 2024

To make this work, a package can configure package.json with

{
	"tsconfigs": ["tsconfig.non-dom.json", "tsconfig.dom.json"]
}

These must be named like tsconfig.*.json. They must not start with ./ or anything and must be present in tsX.Y dirs.

This still works less than optimally because ts-eslint needs to be able to match a tsconfig for every file, which means the main tsconfig still has to include every file. This means that if you want a tsconfig that excludes a file, you need three tsconfigs in order to still have one with everything.

e.g., node has DOM and non-dom; if you want to test both without using expect's ||, then you need to have two differing test files, but then that means three tsconfigs: one with both, then two with one of each.

TODO:

  • dt-mergebot needs to know about these files somehow
    • Just check for tsconfig.*.json.
  • Is this really the right configuration method? Not package.json?

@jakebailey
Copy link
Member Author

Updated to not use settings, and instead specify tsconfigs in package.json. For sake of dt-mergebot, I'm also going to update things to say that the tsconfigs need to have a specific prefix if not named tsconfig.json.

@jakebailey jakebailey marked this pull request as ready for review March 5, 2024 18:20
const tsconfigPath = joinPaths(dirPath, tsconfig);
const tsconfigErrors = checkTsconfig(dirPath, getCompilerOptions(tsconfigPath));
if (tsconfigErrors.length > 0) {
errors.push("\n\t* " + tsconfigErrors.join("\n\t* "));
Copy link
Member

Choose a reason for hiding this comment

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

Is it clear from the error text which tsconfig this applies to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, definitely not, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Modified this to include a filename before each of these groups.

@jakebailey
Copy link
Member Author

I'm going to hold off on merging this because I want to finish a PR that would work for Node.

@jakebailey
Copy link
Member Author

This is all working; I have not done the dt-mergebot change, but since that testing requires an example PR, I'll just merge this, make my node PR ready, and then use that to update dt-mergebot.

@jakebailey
Copy link
Member Author

Also, copying my thoughts on this from the discord:

The main frustration will be that the main tsconfig (used for ts-eslint) needs to include all files, so all of those have to be included. Do you know if these packages would cause big problems if pulled into one project?

If they don't, then it's pretty simple and we can just have multiple tsconfigs with one-off test files that individually /// reference each package as a devDep

I guess the reality is that no matter what a package needs, it'll always be the case that we want some array of tsconfigs to test, so my PR is probably fine. I think the only other things to change would be running a single tsconfig only on a subset of supported versions (so, we can make tsconfigs an object, or an array of objects, or something), and the ability to share the config between tsconfigs (which I want to start supporting anyway to reduce config duplication on DT)

So, I will probably just get this in

@jakebailey jakebailey merged commit adfd769 into microsoft:main Mar 8, 2024
7 checks passed
@jakebailey jakebailey deleted the extra-tsconfig branch March 8, 2024 17:59
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.

2 participants