-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
There was a problem hiding this 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.
Thanks, the failing build seems to be yarn generating different hashes on Windows and Linux |
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 leverageIt's still needed becauseincludeUnusedProps
and filter by filename inshouldInclude
. In the end we'll probably filter out any prop type that's not declared in the same file as the component.includeUnusedProps
causesusedProps
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
wastrue
even though we might have included it later viashouldInclude
@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.