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

Upgrade all dependencies #276

Merged
merged 5 commits into from Aug 26, 2018

Conversation

Projects
None yet
3 participants
@nkbt
Copy link
Contributor

commented Aug 17, 2018

It is WIP, since I wanted to check that all tests pass in CI (everything passes and works locally on my machine, I checked every script that CI runs) without updating code to match new eslint/airbnb
All code is updated too, linting enabled back

  • Switch all old babel-* deps to new @babel/*
  • Replace deprecated packages with new ones
    • babel-preset-* ->@babel/preset-env
    • codecov.io -> codecov
    • babel-node -> node -r @babel/register
  • Upgrade all other dependencies to the most recent versions
  • Deprecate Node4 (many deps are incompatible now anyway) and add Node10
  • Update code to pass eslint + airbnb and uncomment lint check in CI. Done in a separate commit, so all changes can be easily diffed
@codecov

This comment has been minimized.

Copy link

commented Aug 17, 2018

Codecov Report

Merging #276 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #276   +/-   ##
=======================================
  Coverage   98.04%   98.04%           
=======================================
  Files          29       29           
  Lines         461      461           
=======================================
  Hits          452      452           
  Misses          9        9
Impacted Files Coverage Δ
src/parser/typescript.js 83.33% <ø> (ø) ⬆️
src/detector/importCallExpression.js 100% <ø> (ø) ⬆️
src/cli.js 100% <ø> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/detector/importDeclaration.js 100% <100%> (ø) ⬆️
src/detector/gruntLoadTaskCallExpression.js 100% <100%> (ø) ⬆️
src/detector/expressViewEngine.js 100% <100%> (ø) ⬆️
src/detector/requireResolveCallExpression.js 100% <100%> (ø) ⬆️
src/check.js 98.83% <100%> (ø) ⬆️
src/special/gulp-load-plugins.js 96.55% <100%> (ø) ⬆️
... and 4 more

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 0a11cf8...77e2117. Read the comment docs.

@nkbt nkbt changed the title [wip] Upgrade all dependencies Upgrade all dependencies Aug 17, 2018

@@ -49,12 +49,12 @@ function hasVisited(ast, visited) {
function recursive(ast, visited) {
if (!ast || hasVisited(ast, visited)) {
return [];
} else if (lodash.isArray(ast)) {
} if (lodash.isArray(ast)) {

This comment has been minimized.

Copy link
@LinusU

LinusU Aug 17, 2018

Member

Not a fan of this style, can we add some newlines?

 if (!ast || hasVisited(ast, visited)) {
   return [];
-} if (lodash.isArray(ast)) {
+}
+
+if (lodash.isArray(ast)) {

This comment has been minimized.

Copy link
@nkbt

nkbt Aug 17, 2018

Author Contributor

Sure, all changes except maybe around 10 were eslint . --fix

' when it is `false`. This option will be removed and enforced to `true` in next' +
' major version.',
'The option `dev` is deprecated. It leads a wrong result for missing dependencies'
+ ' when it is `false`. This option will be removed and enforced to `true` in next'

This comment has been minimized.

Copy link
@LinusU

LinusU Aug 17, 2018

Member

personally not a fan of this, did the AirBnB style guide switch? did they give a motivation as to why? 🤔

This comment has been minimized.

Copy link
@nkbt

nkbt Aug 17, 2018

Author Contributor

Probably, I did not follow all their changes, just applied --fix. I used to be more involved with the code style stuff at some point, but found that I care less and less over time, as soon as it is automatic and"uniform" across the codebase

test/cli.js Outdated
.then(({ log, error, exitCode }) => {
error.should.containEql('/not/exist/folder').and.containEql('not exist');
log.should.be.empty();
exitCode.should.equal(-1);
}));

it('should output call stack for invalid files in JSON view', () =>
testCli(makeArgv('bad_js', { json: true }))
it('should output call stack for invalid files in JSON view', () => testCli(makeArgv('bad_js', { json: true }))

This comment has been minimized.

Copy link
@LinusU

LinusU Aug 17, 2018

Member

Maybe we could change this into async functions?

This comment has been minimized.

Copy link
@nkbt

nkbt Aug 17, 2018

Author Contributor

I guess it is out of scope for this PR, though I'm surely all for async/await these days.

@mnkhouri

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Thanks for taking this on @nkbt! Fixes #273

@nkbt nkbt force-pushed the nkbt:upgrade-dependencies branch from 19d5579 to c7782cb Aug 21, 2018

@nkbt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

  • Fixed eslint autofix for } if () {
  • Rebased
},
},
})));
testCases.forEach(testCase => it(`should ${testCase.name} inside package.json file env section`, () => testBabel('package.json', testCase.deps, {

This comment has been minimized.

Copy link
@mnkhouri

mnkhouri Aug 22, 2018

Member

The airbnb style calls for implicit returns on the same line (as seen here).

However, based on how the tests are written in this repo (with lots of nested implicit returns), that option makes them unreadable, so I think we should change the setting to "implicit return below"

P.S. Sorry that this PR is turning into a style / eslint config discussion @nkbt 😞 I know it's a chore (but I agree that it's better to have the linter running / enforced!)

This comment has been minimized.

Copy link
@nkbt

nkbt Aug 22, 2018

Author Contributor

No problem, I'll fix that

I would actually prefer to have prettier in place and just do not even think about style anymore. Basically as a common style that everyone equally dislikes but since it is automatic and you don't need to care about it - agree to follow =)

This comment has been minimized.

Copy link
@nkbt

nkbt Aug 22, 2018

Author Contributor

I turned off that rule since there is a lot of code that follows either way

@nkbt nkbt force-pushed the nkbt:upgrade-dependencies branch 2 times, most recently from 85f7704 to 2640f54 Aug 22, 2018

@nkbt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

Reverted, turned off rule, re-applied autofix, manually applied some extra fixes for what linter could not do.

@mnkhouri

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Thank you for getting this done, feels good to see the deps updated 😅

I'll get this merged in the next few days, right after we get in the two bugfixes needed to release 0.6.10, #279 and #282 (thanks to your issue report on that one :)

@nkbt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

Perfect, thank you!

@mnkhouri

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

@nkbt could you please also update the README.md line "depcheck needs node >= 4"?

@nkbt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

Sure thing. Will try to get to my laptop this weekend =)

@nkbt nkbt force-pushed the nkbt:upgrade-dependencies branch from 2640f54 to 77e2117 Aug 24, 2018

@nkbt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

Updated.

@mnkhouri mnkhouri merged commit 539c6dd into depcheck:master Aug 26, 2018

4 checks passed

codecov/patch 100% of diff hit (target 98.04%)
Details
codecov/project 98.04% (+0%) compared to 0a11cf8
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mnkhouri

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

Thank you @nkbt ! 🎉

@mnkhouri mnkhouri referenced this pull request Aug 26, 2018

Closed

Dependencies out of date #273

@nkbt nkbt deleted the nkbt:upgrade-dependencies branch Aug 26, 2018

@nkbt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

Great, thank you!

mnkhouri added a commit that referenced this pull request Aug 28, 2018

Merge branch 'master' into re-export
* master:
  Upgrade all dependencies (#276)
  Fix error when Typescript is not installed (#282)
  Enable ES6/7 syntax in Typescript (#258)
  Update NPM tokens for deploy from Travis (#278)
  Recognize object arrays in Webpack module.rules.loaders (#233)
  Add support for import() expressions (#205)
  Bump Mocha to 5.x (#274)
  Fix ignore-bin-package default in ReadMe (#252)
  Fix typo: pasers -> parsers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.