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

prepare repo for publishing to npm #1812

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

prepare repo for publishing to npm #1812

wants to merge 2 commits into from

Conversation

topocount
Copy link
Collaborator

@topocount topocount commented Dec 20, 2019

This repo contains a bunch of changes to prepare us to publish to npm.

  • The first and biggest change is renaming @tps to @autarklabs, which is an org name we actually own in the npmjs registry.
  • Second, I updated all the prepublishOnly scripts in the apps to compile the contracts and extract the abi into its own folder, like the A1 apps do. It doesn't appear that apps are used as a dependency for their frontends, so this is not included in the npm release process. Let me know if you have any questions about this.
  • Lastly I made some cosmetic changes to package.json. One was cleaning up the root build:app script. It now leverages a single lerna call rather than a lerna call for each app. A bunch of alphabetization changes were automatically made when I added in the abi-extract tool as a dev-dependency, and I figured I'd let those ride. I can split those out into a separate commit if people feel strongly about it though

@topocount topocount requested a review from a team as a code owner December 20, 2019 14:18
@ghost ghost requested review from ottodevs and Quazia and removed request for a team December 20, 2019 14:18
@topocount topocount requested a review from a team as a code owner January 13, 2020 14:27
@ghost ghost removed their request for review January 13, 2020 15:01
@stellarmagnet stellarmagnet added this to the Offsite Hackathon milestone Jan 28, 2020
Copy link
Member

@ottodevs ottodevs left a comment

Choose a reason for hiding this comment

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

Some conflicts need to be solved, there is a missing prePublishOnly script and some minor potential changes if those make sense.

@@ -14,7 +15,7 @@
"ganache-cli:test": "sh ../../node_modules/@aragon/test-helpers/ganache-cli.sh",
"lint": "solium --dir ./contracts",
"precommit": "lint-staged",
"prepublishOnly": "truffle compile",
"prepublishOnly": "truffle compile --all && npm run abi:extract -- --no-compile",
Copy link
Member

Choose a reason for hiding this comment

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

--all seems redundant here, we have been testing without the option in the about app and it just works fine.

Copy link
Member

Choose a reason for hiding this comment

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

for consistency we may want to call the compile script, as in the rest of apps, anyway not a big deal

Suggested change
"prepublishOnly": "truffle compile --all && npm run abi:extract -- --no-compile",
"prepublishOnly": "npm run compile && npm run abi:extract -- --no-compile",

},
"scripts": {
"abi:extract": "truffle-extract --output abi/ --keys abi",
Copy link
Member

Choose a reason for hiding this comment

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

prePublishOnly script is missing here

@@ -11,7 +11,7 @@
ℹ Transaction hash: 0x4231262285244a475668e8dda953917a64cb9367885c822ca02eea3cae69bac4
```

- command: `./node_modules/.bin/lerna exec --scope="@tps/apps-address-book" --stream aragon apm publish patch -- --files dist/ --environment staging`
- command: `./node_modules/.bin/lerna exec --scope="@autarklabs/apps-address-book" --stream aragon apm publish patch -- --files dist/ --environment staging`
Copy link
Member

Choose a reason for hiding this comment

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

This document is for historic purposes, so that is the command that was used for that specific deployment, but I guess is ok to change it here anyway

@@ -22,7 +22,7 @@
ℹ Transaction hash: 0x0b4bef766abb49543810a242902fe9cea3d534184f8001ade47f341559982c2b
```

- command: `./node_modules/.bin/lerna exec --scope="@tps/apps-address-book" --stream aragon apm publish major -- --files dist/ --environment staging`
- command: `./node_modules/.bin/lerna exec --scope="@autarklabs/apps-address-book" --stream aragon apm publish major -- --files dist/ --environment staging`
Copy link
Member

Choose a reason for hiding this comment

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

same here and for the rest of replacements in this file, it is documenting deployments that already happened

@ottodevs ottodevs assigned topocount and unassigned ottodevs Feb 5, 2020
Copy link
Member

@ottodevs ottodevs left a comment

Choose a reason for hiding this comment

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

There are some pending things that we can do separately, but they are still needed to publish on npm, possibly the most needed change is to add the files section to each package.json that will define which files will be part of the npm package (look for example how aragon-apps does that), the other things are more secondary, like adding changelogs or resetting the package versions to 0.0.0 (so they start on 0.0.1 on the next release) or alternatively making them coincide with the deployed versions.

I already experimented with these changes while publishing token-manager and whitelist-oracle.
Anyway, at this moment I think this PR is enough to merge as a foundational work and then do the changes I comment in a separate PR once we agree on them, so just commenting here to keep track

@e18r e18r assigned e18r and topocount and unassigned topocount and e18r Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants