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

refactor(example): use barrel import for auth module #1808

Merged
merged 2 commits into from
May 5, 2019

Conversation

goenning
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[X] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

related to #1684

What is the new behavior?

This PR introduces barrel imports for auth module and its sub-folders.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

While doing this change, I noticed some unused variables on auth specs. This PR also removes them. If this is not OK, I can issue a separate PR just for that.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 28, 2019

Preview docs changes for 4e9954c at https://previews.ngrx.io/pr1808-4e9954c/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!
I think we should also create an index file for auth/models?

What about also exporting the components/containers/effects as an array? Not saying we should, just throwing it here 😅

image
(Taken from Todd Motto)

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 1, 2019

Preview docs changes for 4aff96c at https://previews.ngrx.io/pr1808-4aff96c/

@goenning
Copy link
Contributor Author

goenning commented May 1, 2019

Good point, I've add barrel imports to auth/models now.

As for the array of components, IMHO that'd be too much for the amount of components the example app has. I imagine it could also be confusing for newcomers who are not used to that.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

LGTM
fyi, I have triggered another build for the failing docs

@timdeschryver
Copy link
Member

timdeschryver commented May 3, 2019

🤦‍♂️ because of the cleared cache, we now have the same build problem as in #1795 (comment) ( will be solved once #1795 gets merged)

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.

4 participants