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

Support yarn and pnpm@4 and upgrade deps including the godforsaken linter #365

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Artoria2e5
Copy link

@Artoria2e5 Artoria2e5 commented Oct 18, 2019

This is built upon @vladimyr's yarn commit. To make things smoother I centralized the PM handling.

Some dependencies are upgraded because it looks like they should. These include depcheck and lodash with their respective vulnerbilities, as well as the xo linter because linters should be kept up-to-date. Although I have to say I disagree with plenty of xo's formatting suggestions...

I have refactored the PM handling into one place through a thing called
pm-speaker. It's like Lorax, but for package managers.

Doing so should make things a lot easier when we want to support new
PMs like pnpm@4.
and make sure we are linting everything (**) in lib
@Artoria2e5 Artoria2e5 force-pushed the cli-refact branch 2 times, most recently from ffe9208 to ba01435 Compare October 19, 2019 03:32
@wmertens
Copy link

This is great, without it pnpm@4 fails to install

The exit code 1 was previously used to indicate both "update available"
and error. This messes up a bunch of CI tests.

Now the "error" case is moved to exit code 2.
@@ -5,23 +5,26 @@ const ora = require('ora');
const _ = require('lodash');

function skipUnused(currentState) {
return currentState.get('skipUnused') || // manual option to ignore this
Copy link
Author

Choose a reason for hiding this comment

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

…god. Sometimes linters are just bad at understanding human intent.

@Artoria2e5
Copy link
Author

Artoria2e5 commented Oct 23, 2019

There are a few things I may want to rework in this repo, after this one as well as some other cool stuff gets merged (because they will mess up PR diffs):

  • Convert co+promises to ES8/2017 async/await, because they feel more intuitive for weaklings like me and they can be transpiled.
  • Convert to TypeScript; manually checking for usage and tagging types in my mind was not fun for me. It also provides okay transpiling for the new things.
  • I don't like xo's formatting. I want to turn on its prettier mode, but then it clashes with xo's typescript formatting.

@vladimyr
Copy link

Good luck 🤞 and big 👍 for your effort.
Hopefully it will get picked up and merged...

That being said don't convert it to refactor everything pr especially not before you get some sort of signal from maintainers that they are interested in merging it.

@Artoria2e5
Copy link
Author

most of the diff is a side effect of updating the linter; it has some crazy new notions about indentation now...

@Artoria2e5 Artoria2e5 changed the title Support yarn and pnpm@4 Support yarn and pnpm@4 and upgrade deps including the godforsaken linter Dec 13, 2019
@Artoria2e5
Copy link
Author

Artoria2e5 commented Dec 13, 2019

@dylang / @LinusU any chance to get this reviewed? I don't want this to become a pile of steaming indent changes that fecks everyone up when it finally does get merged.

@vladimyr
Copy link

Eventually this turned up to be even bigger clusterfuck than I initially envisioned. Not only that your work is sitting here unreviewed but it also got duplicated by: #368 & #371 and there is previous pr #361 that effectively became part of your proposed changes. (linking them for sake of visibility)

As always mess will just continue to grow... 😩

@Artoria2e5
Copy link
Author

Lmao, now we have #378 also being a subset of this PR.

@Artoria2e5
Copy link
Author

Oh god the new PR sets a FIXED version. I fecking hate this.

@ghost ghost force-pushed the cli-refact branch from 3fa65f6 to 96c72a7 Compare February 29, 2020 18:43
@Artoria2e5
Copy link
Author

@LinusU Could you please give this one a look? It it not actually 4000 lines of changes, I promise.

appveyor.yml Outdated Show resolved Hide resolved
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