Skip to content
This repository was archived by the owner on Nov 30, 2021. It is now read-only.

Conversation

@J-Thompson12
Copy link
Contributor

@J-Thompson12 J-Thompson12 commented Sep 3, 2020

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Copy link

Choose a reason for hiding this comment

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

As the original contributor for these, I don't have a strong preference between yarn and npm, but to bring awareness on my choice:

  1. I would strongly push against advocating for any npm module to be installed globally to run these tests (or really, to do any local-repo development)
  2. Yarn will be much, much faster at installing these test sub-packages, especially noticeable on CI environments
  3. If we do use npm, let's commit the package-lock.json :).
  4. Note that npm does not come with its own monorepo capabilities, and so you would have to either script, use lerna, or go into each of the separate packages to install them before they can be run (since they require other dependencies than just truffle).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that yarn was less secure (I'm possibly wrong at this since the last time I used it continuously was in 2017-18), couldn't find any resources that backed this. Performance-wise, yarn is actually better than npm (see here and here). There's a community comparison between the two packages here.

In summary, I think we can undo the changes from this PR and use yarn everywhere (including docs) for the purpose of standardization.

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

I can't build the docs on http://localhost:8080/ using make docs-serve. I think the yarn methods don't require the run command: npm run serve -> yarn serve. Or maybe the yarn.lock file is wrong? afaik there's a cmd to migrate to yarn from npm

Ref: https://nodejs.dev/learn/the-package-json-guide

@J-Thompson12
Copy link
Contributor Author

J-Thompson12 commented Sep 8, 2020

Okay not sure why the existing entities were not working but it works now

@J-Thompson12 J-Thompson12 changed the title remove yarn remove npm Sep 10, 2020
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

tested ACK

@fedekunze fedekunze merged commit 22788e8 into development Sep 22, 2020
@fedekunze fedekunze deleted the justin/npm branch September 22, 2020 08:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants