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

Remove duplicate key from tsconfig.base.json #25664

Conversation

ediamin
Copy link
Contributor

@ediamin ediamin commented Sep 27, 2020

Description

This a very simple PR that removes the duplicate types key in root tsconfig.base.json file.

Current config contains ["react", "node", "requestidlecallback"], and [] as types property value.

{
	"compilerOptions": {
                 ...
		"target": "esnext",
		"types": ["react", "node", "requestidlecallback"],
                ...

		"typeRoots": [ "./typings", "./node_modules/@types" ],
		"types": []
	},
        ...
}

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Hey thanks for working on this. I'm concerned though and I feel that this in not correct, even though there is the duplication of types key.

I'm not an expert for TS and its config, but I believe this compilerOptions would be extracted somewhere and consumed. JS would then keep the last key declared, so now the types: [] overrides the above value.

So we should check to remove the duplicate value, but be sure what occurrence we have to keep.

Refs:
https://www.typescriptlang.org/tsconfig#typeRoots
https://www.typescriptlang.org/tsconfig#types

@ediamin
Copy link
Contributor Author

ediamin commented Sep 28, 2020

Hey @ntsekouras, I'm new to TypeScript and was looking at the Gutenberg's configs today. Then I found the duplicate keys. But now I think I should remove the first key instead of the second key. I guess, since we're using the typeRoots here, we should leave the "types": [] as it is and remove the first one instead. Let's see what the experts say. Then I'll make the changes accordingly.

@sirreal
Copy link
Member

sirreal commented Sep 28, 2020

Thanks for the PR!

… since we're using the typeRoots here, we should leave the "types": [] as it is and remove the first one instead.

This looks correct, can you make the change? I believe the first types is an oversight.

Happy to see you looking at types, I'll take the opportunity to plug #18838 in case you're looking for more opportunities to collaborate 😁

@ediamin
Copy link
Contributor Author

ediamin commented Sep 29, 2020

@sirreal I've updated the code, please review now. And thanks, I'll definitely look at the #18838 👍

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for the help! 👍

@ntsekouras ntsekouras merged commit 15a9cd3 into WordPress:master Sep 29, 2020
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Sep 29, 2020
@sirreal
Copy link
Member

sirreal commented Sep 29, 2020

Thanks!

I'll reference this PR: #21581. It should have removed the types that we cleaned up here.

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.

None yet

3 participants