Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

processors/typescript: depend on files listed in includes in nearest tsconfig.json #17

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

Conversation

christianscott
Copy link
Collaborator

No description provided.

@christianscott christianscott requested a review from joscha as a code owner May 26, 2022 01:58
Copy link
Contributor

@joscha joscha left a comment

Choose a reason for hiding this comment

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

Good addition!

src/processors/typescript.ts Outdated Show resolved Hide resolved
src/processors/typescript.ts Outdated Show resolved Hide resolved
@christianscott christianscott force-pushed the christianscott-includes_from_tsconfig branch 2 times, most recently from 2ddcc39 to 6085e67 Compare May 26, 2022 07:37
@christianscott christianscott force-pushed the christianscott-includes_from_tsconfig branch from 6085e67 to 8641356 Compare May 26, 2022 07:47
return [];
}

const json = ts.parseJsonText(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

much nicer :)

@christianscott christianscott requested a review from joscha May 26, 2022 07:59
expect(result).toStrictEqual({
missing: new Map(),
resolved: new Map([
[fixture('tsconfigs', 'project', 'project.ts'), new Set([decl1])],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +525 to +526
[fixture('tsconfigs', 'decls', 'one.d.ts'), new Set([decl2])],
[fixture('tsconfigs', 'decls', 'two.d.ts'), new Set([decl1])],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this seemed a little odd to me at first but it makes sense. If I have decls/a.d.ts and decls/b.d.ts which uses types from a, then any file that uses types from b should transitively depend on a.

Copy link
Contributor

@joscha joscha left a comment

Choose a reason for hiding this comment

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

Looking great! Almost there!

);
return parsed.fileNames.filter(
(fileName) =>
// only include .d.ts files. all other references should be made using `import` or `require`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think standard ts(x) files can be referenced with types in them? Can you check? Maybe it makes sense for us to not filter this at all and just take the references verbatim?


// TypeScript files can *implicitly* depend on .d.ts files. We discover
// these files by extracting them from the nearest tsconfig.json file.
// These do not need to be processed further since they have already been fully
Copy link
Contributor

Choose a reason for hiding this comment

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

We are sure these are absolute paths at all times?
What happens if a non-existent file is referenced, does it end up in the unresolvable set of files correctly? Did you add a test or if not, could you please?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants