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

Add angular@7 cli tests #4524

Merged
merged 5 commits into from Oct 24, 2018
Merged

Add angular@7 cli tests #4524

merged 5 commits into from Oct 24, 2018

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Oct 23, 2018

Issue: Add angular@7 to cli tests

@igor-dv igor-dv self-assigned this Oct 23, 2018
kroeder
kroeder reviewed Oct 23, 2018
View reviewed changes
lib/cli/test/fixtures/angular-cli-v7/.editorconfig
@@ -0,0 +1,13 @@
# Editor configuration, see http://editorconfig.org
Copy link
Member

Choose a reason for hiding this comment

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

Were there ever changes to a fixture app? I removed .editorconfig in the v6 fixture because I thought it would just apply rules you don't want to apply

But they are re-written by the pre-commit anyway, I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

I can delete this... didn't even notice it =)

Copy link
Member

Choose a reason for hiding this comment

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

They added this file in the generator of ng new - I don't know why.

@codecov
Copy link

codecov bot commented Oct 23, 2018 •
edited

Codecov Report

Merging #4524 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4524   +/-   ##
=======================================
  Coverage   35.58%   35.58%           
=======================================
  Files         557      557           
  Lines        6730     6730           
  Branches      883      883           
=======================================
  Hits         2395     2395           
  Misses       3876     3876           
  Partials      459      459

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d82766...9325735. Read the comment docs.

@igor-dv igor-dv requested a review from alterx as a code owner October 23, 2018 11:21
@igor-dv
Copy link
Member Author

igor-dv commented Oct 23, 2018

After bumping the angular versions something broke in react 🤔

@igor-dv igor-dv added the maintenance User-facing maintenance tasks label Oct 23, 2018
@igor-dv igor-dv mentioned this pull request Oct 23, 2018
Merged
@Hypnosphi
Copy link
Member

Please try merging master

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 23, 2018 •
edited

But it's possible that this is the problem with hoisting I mentioned in #4405 (comment)

You've changed some dependencies in project, it affects node_modules tree

@chadfawcett chadfawcett mentioned this pull request Oct 23, 2018
Merged
@igor-dv
Copy link
Member Author

igor-dv commented Oct 23, 2018

But it's possible that this is the problem with hoisting I mentioned in #4405 (comment)

Yep, also mentioned it in #4405 (comment) =)

@igor-dv
Merge remote-tracking branch 'origin/master' into cli/angular-cli-v7-…
25d16d1 
…test-2

# Conflicts:
#	examples/angular-cli/package.json
@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 23, 2018 •
edited

Friendly reminder: the best way to resolve conflicts in yarn.lock is just to run yarn
yarnpkg/yarn#3544

@igor-dv
Copy link
Member Author

igor-dv commented Oct 24, 2018

TBH, that is what I am doing. If I have conflicts during the merge in yarn.lock, I just take it from the parent and yarn install again 🤷‍♂️

shilman
shilman approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@shilman shilman merged commit 57c9b38 into master Oct 24, 2018
@shilman shilman deleted the cli/angular-cli-v7-test-2 branch October 24, 2018 08:52
@Hypnosphi
Copy link
Member

If I have conflicts during the merge in yarn.lock, I just take it from the parent and yarn install again 🤷‍♂️

That's what I did before I knew about this feature =)

@igor-dv
Copy link
Member Author

igor-dv commented Oct 24, 2018

Wait, you mean I can run yarn with the broken yarn.lock, and it will fix it ? 😵

@Hypnosphi
Copy link
Member

Exactly

@Hypnosphi
Copy link
Member

AFAIK npm@6 does this with package-lock as well

@igor-dv
Copy link
Member Author

igor-dv commented Oct 24, 2018

😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kroeder kroeder kroeder left review comments

@shilman shilman shilman approved these changes

@Hypnosphi Hypnosphi Hypnosphi approved these changes

@ndelangen ndelangen Awaiting requested review from ndelangen ndelangen is a code owner

@stijnkoopal stijnkoopal Awaiting requested review from stijnkoopal stijnkoopal is a code owner

@alterx alterx Awaiting requested review from alterx alterx is a code owner

Assignees

@igor-dv igor-dv

Labels
angular cli maintenance User-facing maintenance tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@igor-dv @Hypnosphi @shilman @kroeder