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

ci(appveyor): Use npm 4, npm 5 not usable on windows #286

Closed
wants to merge 1 commit into from

Conversation

sudo-suhas
Copy link
Collaborator

Closes #274

@sudo-suhas
Copy link
Collaborator Author

I didn't realise that caching was still enabled. Should we just disable the cache and not switch to npm 4?

@codecov
Copy link

codecov bot commented Sep 16, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #286   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files          10       10           
  Lines         168      168           
  Branches       25       25           
=======================================
  Hits          154      154           
  Misses         13       13           
  Partials        1        1

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 d1f2436...6a43dc5. Read the comment docs.

@luftywiranda13
Copy link
Collaborator

Why don't we mimic what we have in our Travis config? I think by updating our appveyor config like this will only save us from some problems, but not our users problems.. We have to know

I'd prefer using the default version of npm from each of node versions we're using in appveyor.

What do you think?

@luftywiranda13
Copy link
Collaborator

I've updated all of my appveyor config, get rid of npm install --global npm (just use default npm version). Everything's great since then BTW

@sudo-suhas
Copy link
Collaborator Author

We have discussed this already right? Default npm won't work on node 4.2 which comes with 2.14.7. IMHO, we test on windows to check that nothing breaks on it. We already have travis to check against npm. Currently appveyor uses npm@latest so I don't see a problem using yarn or npm@4.

If you still think it will be better to mimic travis config, could you please make a PR or just update this one?

@luftywiranda13
Copy link
Collaborator

Sure, I'm gonna finish #283 (comment) first

BTW, In windows, can I use this?
https://github.com/okonet/lint-staged/blob/d1f24364eec31ffaa26afaa3169da9336551cdaf/.travis.yml#L8

eslint-config-okonet needs to be run on npm >=3

@sudo-suhas
Copy link
Collaborator Author

BTW, In windows, can I use this?

I think that's bash. So no.

@okonet
Copy link
Collaborator

okonet commented Sep 16, 2017

If eslint-config-okonet is the problem, let's drop it in favor of XO or standard?

@sudo-suhas
Copy link
Collaborator Author

Haven't used XO but I am not in favor of standard. I have seen some repositories which use it and IMHO it does not enforce some good practices. I personally like being able to configure it to our needs which is not how standard is meant to be used.

@luftywiranda13
Copy link
Collaborator

XO comes with many good defaults. Easier to setup (less dotfiles, no peerDeps to be installed, etc.)

https://github.com/sindresorhus/eslint-config-xo
https://github.com/sindresorhus/eslint-plugin-unicorn
are the magic behind it.

But yeah sometimes it feels slow especially its atom integration

Or we can go with.. #287 (comment)

@@ -12,7 +12,7 @@ environment:
install:
- ps: Install-Product node $env:nodejs_version
- set CI=true
- npm install --global npm@latest
- npm install --global npm@4
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you think it's important, I'm suggesting to just remove npm install --global npm.
So appveyor will use the default version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or if you prefer using yarn here, just go with it.. But i think we're gonna lose our chance to catch npm bugs on Windows that may affect even more users than yarn

@sudo-suhas sudo-suhas closed this Oct 4, 2017
@sudo-suhas sudo-suhas deleted the ci/appveyor-use-npm4 branch October 4, 2017 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants