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(injector): add component to shouldInclude #8

Merged
merged 9 commits into from
Mar 31, 2020

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Mar 26, 2020

Edit: I think I have a better idea that solves the same issue: Let's include the filename where the prop/component symnol is declared. That way we can leverage includeUnusedProps and filter by filename in shouldInclude. In the end we'll probably filter out any prop type that's not declared in the same file as the component. It's still needed because includeUnusedProps causes usedProps to be empty.

Two goals are achieved with this change:

  • includeUnusedProps: true + shouldInclude as a ignore list doesn't scale very well if you extend very large prop types (e.g. HTML Attributes).
    It seemed like it bailed out early if includeUnusedProps was true even though we might have included it later via shouldInclude
  • we want to move from custom @document directives to internal allow lists. Having magic markers in the jsdoc means that library users also see these magic markers.

I've separated this PR into test and implementation in case there's a better implementation (or even existing options) to achieve the same goal.

src/injector.ts Outdated Show resolved Hide resolved
eps1lon referenced this pull request in eps1lon/typescript-to-proptypes Mar 27, 2020
eps1lon referenced this pull request in eps1lon/typescript-to-proptypes Mar 27, 2020
Copy link
Owner

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Left some feedback inline, otherwise it looks good

It seemed like it bailed out early if includeUnusedProps was true even though we might have included it later via shouldInclude
It's still needed because includeUnusedProps causes usedProps to be empty.

Fixed in 277258d since it required some more work and as it is unrelated to this PR.

src/generator.ts Outdated Show resolved Hide resolved
test/injector/should-include-component-based/input.tsx Outdated Show resolved Hide resolved
src/generator.ts Outdated Show resolved Hide resolved
src/generator.ts Outdated Show resolved Hide resolved
src/injector.ts Show resolved Hide resolved
This was referenced Mar 30, 2020
@merceyz merceyz merged commit 18a7fce into merceyz:master Mar 31, 2020
@merceyz
Copy link
Owner

merceyz commented Mar 31, 2020

Thanks, the failing build seems to be yarn generating different hashes on Windows and Linux

@eps1lon eps1lon deleted the feat/shouldInclude-component branch March 31, 2020 09:19
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