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: lookup project tsconfig #2068

Merged
merged 1 commit into from
Sep 30, 2024
Merged

feat: lookup project tsconfig #2068

merged 1 commit into from
Sep 30, 2024

Conversation

slhck
Copy link
Contributor

@slhck slhck commented Sep 9, 2024

This looks up the default tsconfig.json file for the current project when used from the CLI, to avoid possible compliation errors. Fixes #2062

@arthurfiorette
Copy link
Collaborator

please create a test for when no tsconfig files can be found

@arthurfiorette arthurfiorette reopened this Sep 9, 2024
@arthurfiorette arthurfiorette self-assigned this Sep 9, 2024
@slhck
Copy link
Contributor Author

slhck commented Sep 9, 2024

If no tsconfig.json file is found, the behavior is exactly the same as before this commit — you can see the only change is the assignment in the arguments. Since you have no real CLI tests suite, I'm not sure how this could be easily integrated.

@slhck
Copy link
Contributor Author

slhck commented Sep 9, 2024

(Or do you mean just in the unit test, because of the missing code coverage for the "not found" case?)

@slhck
Copy link
Contributor Author

slhck commented Sep 9, 2024

Please check if the new tests suffice.

* @param filename - The filename of the tsconfig.json file. Defaults to "tsconfig.json".
* @returns The path to the tsconfig.json file or undefined if no such file was found.
*/
export function findTsConfig(directory: string = process.cwd(), filename = "tsconfig.json"): string | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance that we can import this logic from tsc rather than implementing it? If not, then this is good.

@arthurfiorette
Copy link
Collaborator

Maybe (to offload this logic from this codebase) https://www.npmjs.com/package/get-tsconfig?activeTab=readme could be a good idea.

@domoritz
Copy link
Member

Oh look at the bottom of the read me. They show how to use tsc.

@slhck
Copy link
Contributor Author

slhck commented Sep 10, 2024

Since you don't depend on tsc, I don't think that is a viable option, unless you assume users will always have tsc present in their modules.

The third-party package you linked to looks like another option (there's a package for everything in Node these days, huh), but it means having another depdendency (with its own dependencies) for something rather trivial, and you don't need its other parsing features. In essence, it does exactly the same as the function I committed.

Feel free to merge or just replace the one function call with the third-party dependency — I am not passionate about this.

@domoritz
Copy link
Member

We depend on typescript.

And yes I agree, no to another dep.

@slhck
Copy link
Contributor Author

slhck commented Sep 11, 2024

We depend on typescript.

Where?

"dependencies": {
"@types/json-schema": "^7.0.15",
"commander": "^12.1.0",
"glob": "^10.4.2",
"json5": "^2.2.3",
"normalize-path": "^3.0.0",
"safe-stable-stringify": "^2.4.3",
"tslib": "^2.6.3",
"typescript": "^5.5.2"
},
"devDependencies": {
"@auto-it/conventional-commits": "^11.1.6",
"@auto-it/first-time-contributor": "^11.1.6",
"@babel/core": "^7.24.7",
"@babel/preset-env": "^7.24.7",
"@babel/preset-typescript": "^7.24.7",
"@eslint/js": "^9.6.0",
"@types/eslint": "^9.6.0",
"@types/glob": "^8.1.0",
"@types/jest": "^29.5.12",
"@types/node": "^22.0.0",
"@types/normalize-path": "^3.0.2",
"ajv": "^8.16.0",
"ajv-formats": "^3.0.1",
"auto": "^11.1.6",
"chai": "^5.1.1",
"cross-env": "^7.0.3",
"eslint": "^9.6.0",
"eslint-config-prettier": "^9.1.0",
"eslint-plugin-prettier": "^5.1.3",
"jest": "^29.7.0",
"jest-junit": "^16.0.0",
"prettier": "^3.3.2",
"tsx": "^4.16.0",
"typescript-eslint": "^7.14.1",
"vega": "^5.30.0",
"vega-lite": "^5.19.0"
},

@slhck
Copy link
Contributor Author

slhck commented Sep 11, 2024

Oh, never mind, I was looking in the wrong place. Should I update the PR or could you make that two line change yourself?

@domoritz
Copy link
Member

Go ahead!

@arthurfiorette
Copy link
Collaborator

Oh look at the bottom of the read me. They show how to use tsc.

How tf i have never seen this 🤡

@domoritz
Copy link
Member

Can you refactor the last bit @slhck?

This looks up the default tsconfig.json file for the current project when used from the CLI, to avoid possible compliation errors. Fixes vega#2062
@slhck
Copy link
Contributor Author

slhck commented Sep 30, 2024

Please see the new commit.

@swift502
Copy link

I got burned on this last week, happy to see it getting improved. 👏

@domoritz
Copy link
Member

Thank you!

@domoritz domoritz enabled auto-merge (squash) September 30, 2024 12:42
@domoritz domoritz merged commit 83e31c5 into vega:next Sep 30, 2024
2 checks passed
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.

Clarify documentation on tsconfig usage
4 participants