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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependencies #2909

Merged
merged 17 commits into from Mar 6, 2018

Conversation

@josepjaume
Copy link
Contributor

commented Mar 5, 2018

馃帺 What? Why?

Updates all dependencies for fun & profit.

馃搶 Related Issues

None

馃搵 Subtasks

None

馃摲 Screenshots (optional)

None

@ghost ghost assigned josepjaume Mar 5, 2018
@ghost ghost added the in-progress label Mar 5, 2018
@codecov

This comment has been minimized.

Copy link

commented Mar 5, 2018

Codecov Report

Merging #2909 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2909      +/-   ##
==========================================
+ Coverage   98.64%   98.72%   +0.08%     
==========================================
  Files        1627     1656      +29     
  Lines       38784    39458     +674     
==========================================
+ Hits        38257    38956     +699     
+ Misses        527      502      -25
@josepjaume

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

@deivid-rodriguez it turns out that yarn was messing with dependencies in a way that typescript couldn't compile (something like "i've imported the same type from different versions, so 馃挬 "). It turns out that switching to npm fixes the issue.

There's little value in using yarn nowadays - npm has gotten real fast, features package-lock.json by default, and they seem to be doing great work lately. So I've made the switch - what do you think about this?

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

Funny how we switch back & forth between them because of hitting issues that the competition has solved! It's great, it means there's a healthy and tight competition, and that's good for everyone :)

I'm pretty sure the issue that made me propose the last switch to yarn last time was solved with npm 5.6.0. Let me quickly checkout this PR and confirm that.

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

Yeah, checked it out, run npm install on my machine (Linux), and got no local changes in package-lock.json (which I assume you generated on macOS). So multiplatform support seems good now! 馃憤

josepjaume added 9 commits Mar 5, 2018
@josepjaume josepjaume force-pushed the update_dependencies branch from ab54abd to 17a4af2 Mar 5, 2018
josepjaume added 8 commits Mar 5, 2018
@josepjaume josepjaume merged commit 91e0a84 into master Mar 6, 2018
21 checks passed
21 checks passed
ci/circleci: accountability Your tests passed on CircleCI!
Details
ci/circleci: admin Your tests passed on CircleCI!
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: assemblies Your tests passed on CircleCI!
Details
ci/circleci: budgets Your tests passed on CircleCI!
Details
ci/circleci: build_design_app Your tests passed on CircleCI!
Details
ci/circleci: build_test_app Your tests passed on CircleCI!
Details
ci/circleci: comments Your tests passed on CircleCI!
Details
ci/circleci: core Your tests passed on CircleCI!
Details
ci/circleci: debates Your tests passed on CircleCI!
Details
ci/circleci: main Your tests passed on CircleCI!
Details
ci/circleci: meetings Your tests passed on CircleCI!
Details
ci/circleci: pages Your tests passed on CircleCI!
Details
ci/circleci: processes Your tests passed on CircleCI!
Details
ci/circleci: proposals Your tests passed on CircleCI!
Details
ci/circleci: surveys Your tests passed on CircleCI!
Details
ci/circleci: system Your tests passed on CircleCI!
Details
ci/circleci: verifications Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 98.72% (+0.08%) compared to b741f2b
Details
@josepjaume josepjaume deleted the update_dependencies branch Mar 6, 2018
@ghost ghost removed the in-progress label Mar 6, 2018
Copy link
Contributor

left a comment

I'm a bit confused because some changes add a trailing comma and some remove it. I guess the additions were unintended but in any case, I think this kind of global stylistic changes should be discussed separately.

@@ -27,7 +27,7 @@ def call
return broadcast(:invalid) if form.invalid?

ActiveRecord::Base.transaction do
create_or_invite_user
@user = @user ||= existing_user || new_user

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Mar 6, 2018

Contributor

Unintended?

This comment has been minimized.

Copy link
@oriolgual

This comment has been minimized.

Copy link
@josepjaume

josepjaume Mar 6, 2018

Author Contributor

Lol, definitely unintended. Seems to work though xDDD. Will fix in a further PR

This comment has been minimized.

Copy link
@josepjaume

josepjaume Mar 6, 2018

Author Contributor

@deivid-rodriguez we have inconsistencies between tslint and eslint. I plan to move all this to prettier (that supports both file formats) in a further PR but unfortunately I don't have time right now to deal with it. I'll try to do it soon, though!

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Mar 6, 2018

Contributor

Although they are different syntaxes, I guess it makes sense to normalize common rules 馃憤. But please do it in a separate PR so we can discuss it instead of sneaking it in a ~ +13K, -7K one! 馃槅

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Mar 6, 2018

Contributor

Lol, definitely unintended. Seems to work though xDDD. Will fix in a further PR

I've actually seen a similar technique somewhere to avoid redefinition warnings, so I wasn't sure it wasn't some trick of yours 馃槃

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Mar 6, 2018

Contributor

I plan to move all this to prettier (that supports both file formats) in a further PR but unfortunately I don't have time right now to deal with it. I'll try to do it soon, though!

Sorry for triple posting, but... could you actually open a ticket first for discussion? I haven't hit any walls with eslint + tslint, and moving to prettier would imply reconfiguring my editor to play nice with it or stop having JS syntax checking until I do so, so I'd like to hear some motivations before the change!

}
"process.env": {
NODE_ENV: ifProd('"production"', '"development"'),
},

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Mar 6, 2018

Contributor

These are the trailing commas I was refering to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.