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

Tooling: Improve docs build to consider memoized selectors #9756

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 10, 2018

Previously: #7264

This pull request seeks to improve our docs parser to consider memoized selectors. These were previously omitted because they aren't technically FunctionDeclaration, and our logic thus omitted them. It was assumed the omission is based on some files including other non-selector/actions exports which are not intended to be documented as if they were. With these changes is an attempt to preserve this intent while allowing for memoized selectors to be included in the parsed documentation. This is done by considering a selector as one which has a DocBlock containing at least one @param tag, which we assume as always being the case with state as the first argument.

Testing instructions:

Ensure no changes, errors, or included non-selector/actions with npm run docs:build.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Type] Developer Documentation Documentation for developers [Type] Build Tooling Issues or PRs related to build tooling labels Sep 10, 2018
@aduth aduth requested a review from youknowriad September 10, 2018 15:32
@youknowriad
Copy link
Contributor

What if we export just a const (to be used in reducers or whatever), will it be caught by this update? should we target memoized selectors explicitly (createSelector calls)

@youknowriad
Copy link
Contributor

Ok I just saw your exclusion rule :)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants