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

fix: Inconsistent named feature arrays. #69

Merged
merged 4 commits into from
Jan 7, 2019
Merged

fix: Inconsistent named feature arrays. #69

merged 4 commits into from
Jan 7, 2019

Conversation

m-abs
Copy link
Contributor

@m-abs m-abs commented Jan 4, 2019

Fixes: #68

@m-abs
Copy link
Contributor Author

m-abs commented Jan 4, 2019

The tests are failing.

I hope to fix it over the weekend.

@@ -546,7 +546,7 @@ export function adjustBarrelIndex(
}
}

if (type === "component" || type === "service") {
if (type === "component" || type === "service" || type === "directive" || type === 'pipe') {
Copy link
Member

Choose a reason for hiding this comment

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

@m-abs thank you for this PR and good catch. This is the only change I’m not so sure about. There’s really never a case I’ve run into where I need the directive or pipe symbol exported from the barrel which is why this was just component and service since those are often used for various reasons. Curious what your reasons are for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly it was to be consistent with components, like here: https://github.com/nstudio/xplat/blob/master/src/feature/_nativescript_component_files/components/index.ts#L7

We sometimes use pipes in code, so it makes sense to have those exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from directives but kept it for pipes.

Copy link
Member

Choose a reason for hiding this comment

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

@m-abs perfect thanks! That makes sense for pipes yeah since can be used as provider where needed.

@NathanWalker NathanWalker merged commit e664ce6 into nstudio:master Jan 7, 2019
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.

3 participants