Skip to content
This repository was 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
christianscott wants to merge 6 commits intomainfrom
christianscott-includes_from_tsconfig
Open

processors/typescript: depend on files listed in includes in nearest tsconfig.json#17
christianscott wants to merge 6 commits intomainfrom
christianscott-includes_from_tsconfig

Conversation

@christianscott
Copy link
Copy Markdown
Contributor

No description provided.

@christianscott christianscott requested a review from joscha as a code owner May 26, 2022 01:58
Copy link
Copy Markdown
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!

Comment thread src/processors/typescript.ts Outdated
Comment thread src/processors/typescript.ts Outdated
Comment thread src/processors/typescript.ts Outdated
@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
Copy Markdown
Contributor 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
Comment thread src/processors/typescript.ts Outdated
Comment thread __tests__/index.test.ts
expect(result).toStrictEqual({
missing: new Map(),
resolved: new Map([
[fixture('tsconfigs', 'project', 'project.ts'), new Set([decl1])],
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread __tests__/index.test.ts
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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