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

fix: use get-tsconfig to handle tsconfig file resolution #247

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

Conversation

effervescentia
Copy link

@effervescentia effervescentia commented Apr 4, 2023

hey @jonaskello I rewrote my other PR using the library get-tsconfig which does not rely on the API exposed by typescript, it has zero dependencies and implements all of the features of the latest TSConfig files (including extends as an array, which I see was a recent change to this repo).

I'd really appreciate some feedback on how to move either of these changes forward

"ts-node": "^10.7.0",
"typescript": "^4.5.2"
"typescript": "^5.0.3"
Copy link
Author

Choose a reason for hiding this comment

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

need to update typescript because get-tsconfig uses the new .d.cts and .d.mts file extensions which were not supported by TypeScript v4.5.2

"lint-staged": "^10.2.11",
"prettier": "^2.0.5",
"rimraf": "^2.6.2",
"ts-jest": "^27.0.7",
"ts-jest": "^29.1.0",
Copy link
Author

Choose a reason for hiding this comment

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

updating ts-jest for compatibility with typescript

@@ -32,18 +32,17 @@
"eslint-plugin-import": "^2.26.0",
"eslint-plugin-jsdoc": "^39.2.9",
"husky": "^4.2.5",
"jest": "^27.3.1",
"jest": "^29.5.0",
Copy link
Author

Choose a reason for hiding this comment

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

updating jest for compatibility with ts-jest

@effervescentia
Copy link
Author

hey @jonaskello following up in this PR as well just to get your attention. let me know if there's anything I can do to help move either of these changes forward

@effervescentia
Copy link
Author

Hi @Jontem sorry to ping you as well, but I wasn't getting a response from your colleague
can you provide any information on how I can progress either this PR or my other PR?

Unfortunately your library is at the root of many typescript-enabled workflows and this issue is seriously affecting my team because it does not follow the extension behaviour of official TypeScript tools

@jonaskello
Copy link
Member

Unfortunately your library is at the root of many typescript-enabled workflows

Yes, I know this :-). So the main problem or merging anything is that we don't want to break any of the many existing users, which makes larger changes like this difficult to assess. I saw that a bunch of tests were removed. I guess this was because the assumption is that the external library is fully tested? But perhaps we can keep all current tests to ensure some level of combability?

@effervescentia
Copy link
Author

Thanks for the reply @jonaskello :) sorry for the continual pings on these changes

and sure 👍 I'm happy to re-add the tests for added safety

Do you have any preference between using get-tsconfig and using the internals of typescript? I'll update the PR for whichever one you're more comfortable with using and close the other

@jonaskello
Copy link
Member

I don't like relying on undocumented API:s so although I don't like external dependencies either I guess get-tsconfig is my preferred choice here :-).

@effervescentia
Copy link
Author

Amazing 👍 I'll rework this PR to keep the existing tests as best as possible

@effervescentia
Copy link
Author

Heya @jonaskello long time no chat 😅
After more than a year brewing I've finally had a chance to go back in and re-add the tests I had removed

As you can see it maintains compatibility with the original test suite while also handling multiple levels of extension (the original reason for this PR)

Let me know what else I can do to push this along, I'll try to follow up more regularly going forward

const res = loadTsconfig(
"/root/dir1/tsconfig.json",
(path) => path === "/root/dir1/tsconfig.json",
(_) => `{
Copy link
Author

Choose a reason for hiding this comment

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

since the API from get-tsconfig does not support a resolve callback like this, I opted instead to create entries in the example/ folder for each test case

});
});

describe("getTsconfig", () => {
Copy link
Author

Choose a reason for hiding this comment

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

I extracted these tests which only target the getTsconfig function from get-tsconfig to show that other details like strict are correctly inherited (since this information is not returned from loadSyncDefault)

@jonaskello
Copy link
Member

Nice work! However, it seems the github checks are failing, perhaps you can take a look at it?

@effervescentia
Copy link
Author

Nice work! However, it seems the github checks are failing, perhaps you can take a look at it?

will do 👍

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