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.

Sign up for GitHub

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

Jump to bottom

feat(rule): add use-injectable-provided-in #814

Merged
merged 3 commits into from May 1, 2019
Merged

feat(rule): add use-injectable-provided-in #814

merged 3 commits into from May 1, 2019

Conversation

mohammedzamakhan
Copy link
Contributor

@mohammedzamakhan mohammedzamakhan commented Apr 23, 2019 •
edited

@rafaelss95 rafaelss95 self-requested a review April 23, 2019 00:15
rafaelss95
rafaelss95 requested changes Apr 23, 2019
View reviewed changes
src/useProvidedInRule.ts Outdated Show resolved Hide resolved
src/useProvidedInRule.ts Outdated Show resolved Hide resolved
src/useProvidedInRule.ts Outdated Show resolved Hide resolved
src/useProvidedInRule.ts Outdated Show resolved Hide resolved
src/useProvidedInRule.ts Outdated Show resolved Hide resolved
test/useProvidedInRule.spec.ts Outdated Show resolved Hide resolved
test/useProvidedInRule.spec.ts Outdated Show resolved Hide resolved
test/useProvidedInRule.spec.ts Outdated Show resolved Hide resolved
test/useProvidedInRule.spec.ts Outdated Show resolved Hide resolved
@rafaelss95
Copy link
Collaborator

@mohammedzamakhan I've left some minor comments, please have a look.

rafaelss95
rafaelss95 approved these changes Apr 24, 2019
View reviewed changes
Copy link
Collaborator

@rafaelss95 rafaelss95 left a comment

Choose a reason for hiding this comment

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

LGTM!

@rafaelss95
Copy link
Collaborator

@mohammedzamakhan Can you just change the commit message to feat(rule): add use-injectable-provided-in?

@rafaelss95 rafaelss95 changed the title feat(rule): use providedIn in services feat(rule): add use-injectable-provided-in Apr 24, 2019
@mohammedzamakhan
Copy link
Contributor Author

@rafaelss95 done 👍

@mgechev
Copy link
Owner

mgechev commented Apr 24, 2019

I'll restart travis and merge when the build passes.

@rafaelss95
Copy link
Collaborator

rafaelss95 commented Apr 27, 2019 •
edited

I investigated the reason behind the build failure and it seems that the problem is with node-sass dependency: our travis config (as you may already know) currently runs with Node 8.11.1 and Node 12 (latest stable), however, node-sass v4.11.0 is not compatible with version 12 node.

The fix should have come in sass/node-sass@23c8659, but apparently it hasn't yet been published in npm.

Edit: They just published the v4.12.0 into npm. I've submitted the fix in #818.

@rafaelss95 rafaelss95 mentioned this pull request Apr 27, 2019
Merged
@mgechev mgechev merged commit 656816f into mgechev:master May 1, 2019
@mohammedzamakhan mohammedzamakhan deleted the provided-in branch May 1, 2019 05:47
@keradus keradus mentioned this pull request Nov 26, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Mar 12, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelss95 rafaelss95 rafaelss95 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mohammedzamakhan @rafaelss95 @mgechev