Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix(29118): tsconfig.extends as array#50403

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
navya9singh merged 15 commits intomainfromfix(29118)
Dec 13, 2022
Merged

Fix(29118): tsconfig.extends as array#50403

navya9singh merged 15 commits intomainfromfix(29118)
Dec 13, 2022

Conversation

navya9singh
Copy link
Member

Fixes#29118

thenbe, xiaoxiangmoe, SchroederSteffen, mklueh, fireairforce, fdecampredon, johncmunson, alecmev, kachick, danielbayley, and testfailed reacted with thumbs up emojicherryblossom000, acidoxee, SchroederSteffen, and mklueh reacted with hooray emojiV-ed and ldgrp reacted with eyes emoji
Copy link
Member

@sheetalkamatsheetalkamat left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@sheetalkamatsheetalkamat left a comment

Choose a reason for hiding this comment

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

I will look into other comments later

@@ -300,6 +300,14 @@ namespace ts {
// TODO: check infinite loop
possibleValues = getPossibleValues(option.element);
break;
case "listOrElement":
if (option.element.type === "string"){

Choose a reason for hiding this comment

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

Suggested change
if(option.element.type==="string"){
if(option.element.type==="string"){

navya9singh reacted with thumbs up emoji
options[opt.name] = validateJsonOptionValue(opt, args[i] || "", errors);
i++;
}
else{

Choose a reason for hiding this comment

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

Suggested change
else{
else{

navya9singh reacted with thumbs up emoji
@@ -2986,34 +3029,52 @@ namespace ts {
if (ownConfig.extendedConfigPath) {
// copy the resolution stack so it is never reused between branches in potential diamond-problem scenarios.
resolutionStack = resolutionStack.concat([resolvedPath]);
const extendedConfig = getExtendedConfig(sourceFile, ownConfig.extendedConfigPath, host, resolutionStack, errors, extendedConfigCache);
const result: ExtendsResult = { options:{} };
if(isString(ownConfig.extendedConfigPath)){

Choose a reason for hiding this comment

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

Spaces throughout

navya9singh reacted with thumbs up emoji
i++;
}
}
Debug.fail();

Choose a reason for hiding this comment

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

Suggested change
Debug.fail();
Debug.fail("listOrElement not supported here");

navya9singh reacted with thumbs up emoji
@@ -2513,6 +2529,7 @@ function serializeOptionBaseObject(
const value = options[name] as CompilerOptionsValue;
const optionDefinition = optionsNameMap.get(name.toLowerCase());
if (optionDefinition) {
Debug.assert(optionDefinition.type !== "listOrElement");
Copy link
Member

Choose a reason for hiding this comment

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

Please fix formatting for some of the changes that has been indented by one extra tab

navya9singh reacted with thumbs up emoji
Copy link
Member

@andrewbranchandrewbranch left a comment

Choose a reason for hiding this comment

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

It would be good to add a compiler test that shows the new feature working and doing something useful. It looks like the one changed baseline shows that what used to be an invalid config is now doingsomething, but it’s hard to see that the multiple extended tsconfigs are actually contributing anything.

@andrewbranch
Copy link
Member

Example:

//@Filename: /tsconfig1.json{"compilerOptions":{"strictNullChecks":true}}//@Filename: /tsconfig2.json{"compilerOptions":{"noImplicitAny":true}}//@Filename: /tsconfig.json{"extends":["./tsconfig1.json","./tsconfig2.json"]"files":["./index.ts"]}//@Filename: /index.tsfunctionf(x){}// noImplicitAny errorlety:string;y.toLowerCase();// strictNullChecks error

@sheetalkamat
Copy link
Member

@andrewbranch the compiler tests are hard to add with tsconfig.https://github.com/microsoft/TypeScript/pull/50403/files#diff-0556827a3985d4ffd3ed2e51ac63c9d86ba98ab41dca5c7711fb2750188cbe8a shows the test case where multiple tsconfigs are working together ?https://github.com/microsoft/TypeScript/pull/50403/files#diff-97bc6a439e0f02498deab0f7c919e4e7a2f06c6b4832fd2fc7b042f56b141ff9R111 baseline.

While said that we could add test in non watch mode as well.

@andrewbranch
Copy link
Member

I saw that baseline, but I couldn’t find any indication in the baseline that the options in the extended configs were relevant. I was looking for astrictNullChecks ornoImplicitAny error but didn’t see any. Maybe I missed it in the noise.

There are plenty of compiler tests with tsconfig files, but I don’t know if there are subtle gotchas with them.

@sheetalkamat
Copy link
Member

@navya9singh you probably want to add test likehttps://github.com/microsoft/TypeScript/blob/main/src/testRunner/unittests/tsc/forceConsistentCasingInFileNames.ts with the contents of test files as shown by@andrewbranch for better clarity

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commentedDec 7, 2022
edited
Loading

Heya@DanielRosenwasser, I've started to run the tarball bundle task on this PR atbb0ae2f. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedDec 7, 2022
edited
Loading

Hey@DanielRosenwasser, I've packed this intoan installable tgz. You can install it for testing by referencing it in yourpackage.json like so:

{    "devDependencies": {        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/139933/artifacts?artifactName=tgz&fileId=43D4A075BE928283C15303888F67D4786C1B8B67071DA37D39B422AFBEEC049702&fileName=/typescript-5.0.0-insiders.20221207.tgz"    }}

and then runningnpm install.


There is also a playgroundfor this build and annpm module you can use via"typescript": "npm:@typescript-deploys/pr-build@5.0.0-pr-50403-6".;

@andrewbranch
Copy link
Member

It looks like some changes landed inmain that require you to update something in your unit tests. I’m guessing it’s#51798. You’ll want to manually mergemain into this branch and then take a look at those test files that no longer build, using Sheetal’s PR as a reference for how to update them.

navya9singh reacted with thumbs up emoji

Copy link
Member

@andrewbranchandrewbranch left a comment

Choose a reason for hiding this comment

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

Once the build is green, looks good to merge.

@andrewbranch
Copy link
Member

:shipit:

navya9singh, fguitton, and phippg reacted with heart emojiThundercraft5 reacted with eyes emoji

@navya9singhnavya9singh merged commit7267fca intomainDec 13, 2022
@navya9singhnavya9singh deleted the fix(29118) branchDecember 13, 2022 19:16
@orta
Copy link
Contributor

Very cool

@bradley-marker-ck
Copy link

bradley-marker-ck commentedMar 31, 2023
edited
Loading

Just a heads up if you're sharing atsconfig from a package. It seemstypescript@4.9.5 was not restricted by theexports from thepackage.json file. It is now with@typescript@5.0.3.

Thetsconfig.json file not found error was the last error when runningtsc.

In my case, it was also causingeslint (with@typescript-eslint/parser) to fail with out of memory errors. Since we runeslint beforetsc, it was a few tries before I decided to skipeslint and runtsc and was able to find the problem.

Jerryh001 reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@andrewbranchandrewbranchandrewbranch approved these changes

@RyanCavanaughRyanCavanaughRyanCavanaugh approved these changes

@sheetalkamatsheetalkamatsheetalkamat approved these changes

Assignees

@navya9singhnavya9singh

Labels
Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone
Projects
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tsconfig.extends as array
8 participants
@navya9singh@andrewbranch@sheetalkamat@DanielRosenwasser@typescript-bot@orta@bradley-marker-ck@RyanCavanaugh

[8]ページ先頭

©2009-2025 Movatter.jp