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

Bump jest to 24.8.0 #243

Merged
merged 2 commits into from
Jun 13, 2019
Merged

Bump jest to 24.8.0 #243

merged 2 commits into from
Jun 13, 2019

Conversation

Tbaut
Copy link
Contributor

@Tbaut Tbaut commented Jun 11, 2019

Self explanatory, and more sensible this time :)
Tested successfully the build and functionnalities on android.

Copy link
Contributor

@pmespresso pmespresso left a comment

Choose a reason for hiding this comment

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

it looks like there's two lock files. I think it's generally best practice to choose one or the other (yarn or npm) to avoid inconsistencies.

@Tbaut
Copy link
Contributor Author

Tbaut commented Jun 11, 2019

Do you mean I should remove the yarn.lock? I've had the package-lock.json in previous PRs`

@pmespresso
Copy link
Contributor

As long as it's consistent it should be no problem. Dug a little into past PR's where just the yarn.lock is committed. e.g. https://github.com/paritytech/parity-signer/pull/232/files

@maciejhirsz
Copy link
Contributor

I don't have any strong feelings for or against yarn/npm, so adding whichever lock file on .gitignore and then using the other is fine with me. npm might be a bit easier since we can then remove yarn from required dependencies for the setup, but I reckon it isn't really a blocker for anyone.

Copy link
Contributor

@pmespresso pmespresso left a comment

Choose a reason for hiding this comment

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

LGTM, @Tbaut could you please also add one of the lock files to gitignore in this PR? shouldn't really matter which one :)

@Tbaut
Copy link
Contributor Author

Tbaut commented Jun 13, 2019

I went for removing the yarn.lock and adding it to .gitignore,

@Tbaut Tbaut merged commit f356654 into master Jun 13, 2019
@Tbaut Tbaut deleted the tbaut-bump-jest branch June 13, 2019 12:51
@pmespresso
Copy link
Contributor

ah I just realized the setup_macos.sh script actually runs yarn at the :/

I will change this to npm install in a separate PR

@lexfrl
Copy link
Contributor

lexfrl commented Jun 26, 2019

I went for removing the yarn.lock and adding it to .gitignore,

Consider to read this:
https://yarnpkg.com/blog/2018/06/04/yarn-import-package-lock/

We highly recommend you to delete the package-lock.json file if you decide to use yarn in order to avoid future confusion and possible consistency issues.

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.

4 participants