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

MAINT: Use yarn as package manager #428

Merged
merged 5 commits into from
Aug 4, 2021

Conversation

stevejpurves
Copy link
Collaborator

Using yarn in place of npm as the package manager

  • tested with yarn and resolved a number of issues around package dependencies.
  • updated contributing docs
  • added advisory around use of v7 and possible peer dependency errors
  • updates the docs build
  • updated ci actions
  • updates dependabot

Addresses #426

@stevejpurves
Copy link
Collaborator Author

Leaving this marked as WIP until ci goes green.

@stevejpurves stevejpurves changed the title [WIP] using yarn as package manager [REFACTOR] using yarn as package manager Jul 26, 2021
@stevejpurves
Copy link
Collaborator Author

ok, this is green and everything seems happy. I think this should make our CI checks more reliable 👍 and would love to get this in.

NOTE: if we accept this change, we should just delete all open dependabot PRs and wait on new ones coming in. Existing dependabot PRs are trying to patch package-lock.json which is now gone and we have updated dependencies with the PR anyways, because of the new yarn-lock.json

@moorepants
Copy link
Collaborator

Got these warnings on first install:

warning "@jupyter-widgets/base > @lumino/coreutils@1.8.0" has unmet peer dependency "crypto@1.0.1".
warning "@jupyterlab/theme-light-extension > @jupyterlab/application > @jupyterlab/ui-components@3.0.7" has unmet peer dependency "react@^17.0.1".
warning "@jupyterlab/testutils > ts-jest@25.5.1" has unmet peer dependency "typescript@>=3.4 <4.0".
warning " > jest-puppeteer@4.4.0" has incorrect peer dependency "puppeteer@>= 1.5.0 < 3".
warning " > ts-jest@27.0.4" has incorrect peer dependency "jest@^27.0.0".
warning " > ts-jest@27.0.4" has unmet peer dependency "typescript@>=3.8 <5.0".

@moorepants
Copy link
Collaborator

moorepants commented Jul 26, 2021

When I run yarn run test locally jest isn't found. Should yarn install add jest? Or is there a separate lock file for dev dependencies?

Edit: prettier is missing for yarn run fmt too.

@choldgraf
Copy link
Collaborator

choldgraf commented Jul 26, 2021

Thanks for this PR - we should really get input from @minrk to make sure he is OK with this. (or @minrk feel free to say that you are fine with whatever @stevejpurves is proposing if you don't have the bandwidth to review etc!).

Not to bottleneck the PR but we should give this some time for others to comment before merging it in since it is a large-ish change. What do you say we ask for feedback within a week (either a review, or a "please don't merge until I have time" etc).

docs/contribute.md Outdated Show resolved Hide resolved
@moorepants
Copy link
Collaborator

There is an empty yarn-lock.json file committed along with the two yarn.lock files. Maybe that is unintentional?

@moorepants
Copy link
Collaborator

This looks good to me so far.

  1. I'm not sure how to install the full development dependencies to run the tests.
  2. We'll likely have to check the production build commands on master when we do a release for a full check of the CI.

Thanks @stevejpurves !

stevejpurves and others added 2 commits July 26, 2021 20:39
Co-authored-by: Jason K. Moore <moorepants@gmail.com>
Co-authored-by: Jason K. Moore <moorepants@gmail.com>
@stevejpurves
Copy link
Collaborator Author

@moorepants to your points above

  • yarn-lock.json removed and the docs makefile is now fixed
  • I have addressed a couple of them. I'm tempted to say leave the others, though? (react, typescript) these are our dependencies telling us we are responsible for providing these peer dependencies, but looking at each warning there, I don't think we are using our dependencies in way where we need to install react, typescript, so would be best not to add them.

@stevejpurves
Copy link
Collaborator Author

@moorepants yarn install should install all package dependencies, including dev ones.

I've just removed my node_modules folder here, run yarn install and I can run tests and yarn run fmt ok. I am using yarn v1.22.10. Not sure why you are not able to rung them. I know you said for a clean install but just want to check, did you remove the node_modules and there is not a package-lock.json hanging around.

@moorepants
Copy link
Collaborator

I'll try again.

@moorepants
Copy link
Collaborator

Everything is working now. Not sure what I did wrong before, maybe I didn't delete the node_modules directory first.

@moorepants
Copy link
Collaborator

+1 from me on this for merging.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I also think this is a nice enhancement! Don't mistake my comments above for a lack of enthusiasm!

@stevejpurves
Copy link
Collaborator Author

no worries @choldgraf it's a good point that we're all on different cadences here and need to allow some time for wider contributions :)

@stevejpurves
Copy link
Collaborator Author

Are we good to merge this one folks? @moorepants @choldgraf (I have no merge button :) )

@choldgraf choldgraf changed the title [REFACTOR] using yarn as package manager MAINT: Use yarn as package manager Aug 4, 2021
@choldgraf choldgraf merged commit a658c32 into jupyter-book:master Aug 4, 2021
@choldgraf
Copy link
Collaborator

Sounds good! Thanks for the ping 🙂

@stevejpurves stevejpurves deleted the feat/yarn branch October 4, 2021 08:58
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