Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

One line to rule them all #95

Merged
merged 22 commits into from
Apr 18, 2018
Merged

One line to rule them all #95

merged 22 commits into from
Apr 18, 2018

Conversation

tyleryasaka
Copy link
Contributor

@tyleryasaka tyleryasaka commented Apr 14, 2018 •
edited

Checklist:

  • Code contains relevant tests for the problem you are solving
  • Ensure all new and existing tests pass
  • Update any relevant READMEs and docs

Description:

This is a huge restructure, but I think this moves us in the right direction.

  1. One-line setup! Just npm setup and npm start to get it up and running and linked locally. Bonus: npm run test:contracts to run contract tests.
  2. npm start really does everything:
    • starts ganache
    • deploys contracts using migrations
    • builds JS
    • watches for changes in contracts; change triggers redeployment, which triggers JS rebuild
    • watches for changes in JS; change triggers rebuild
    • makes browser tests automatically available at localhost:8081
  3. Abstracts Truffle behind our own commands. We're still using it as much as we were before, but now we're calling it programmatically from our scripts. If we ever decide to drop Truffle (for example to be able to use the latest web3), we can do so without having to change our development process.
  4. Simpler codebase structure. We now have just one npm package, and it's in the root of the repo. Makes codebase easier to understand and work with imho.
  5. Removes env var dependencies. origin.js will always be injected, so configurable options like IPFS gateway are passed into the constructor at initialization.
  6. Updates readme. I've merged everything into a single readme. There's probably too much in it at the moment. I think the next step would be to move some of the more general Origin Protocol-level stuff to a separate repo (maybe documentation?). This readme should be specific to the origin.js library.

New structure looks like:

  • demo dapp
    • readme (contains the full setup instructions; references platform readme)
  • platform/origin.js (the repo is still named platform, but it exports a single origin.js package at the top level)
    • readme (contains setup instructions just for origin.js)
    • contracts (just a directory, not a package; contains all of the truffle code including the solidity contracts)

Also, see accompanying PR in demo dapp repo. The main change there is that it updates the readme to correspond to these changes. OriginProtocol/origin#108

I've tested the process of walking through local setup using both readmes - but if other people could also try it out that would be great!

This was referenced Apr 14, 2018
Closed
Open
@tyleryasaka tyleryasaka force-pushed the one-line-to-rule-them-all branch 3 times, most recently from 65858f5 to f340b88 Compare April 17, 2018 08:59
@tyleryasaka
Make seed script a migration once more
03e9356 
I've changed my mind on this. Our one-line scripts are much more
manageable if the seeding happens with the migrations.

In addition, I discovered that you can tell the migrations whether or
not to run on a per-network basis. So that takes care of my concern
about running a seed data migration in production.
@tyleryasaka
Merge contracts package into origin.js package
2745fcd 
We'll be using a single command to build/deploy our contracts as well as
our JS. Makes sense and makes the command simpler if all of the code is
in the same directory. Also I think it's just simpler to have a single
package in a repo.
@tyleryasaka
Create one-line start script
b0b1ec1 
Starts ganache, deploys contracts, builds JS, and watches for changes
@tyleryasaka
Move contracts back to their own "package"
1a657b1 
Truffle is very rigid about its tests. It mandates that the truffle
tests are placed in the `test` directory, no exceptions, and it tries to
run all of the tests in that directory. In other words, we cannot put
any origin.js integration tests in that directory. We could work around
this, but at this point it starts getting messy, so I think it's
definitely best to just keep the truffle code separate for now.
@tyleryasaka
Move origin.js to top level
3f9b2d3 
Reasons for doing this:

1. Improving developer experience by allowing them to run `npm install`
from the repo root. We could hack our way around this with the old
scheme, but that gets weird really fast

2. Maintaining and exporting just a single npm package. What will be
consumed is a simple npm package. That's what should be at the top
level.

3. Simpler! This setup is more readable imo.
@tyleryasaka
Refactor index to handle start and build commands
4bd3204
@tyleryasaka
Keep readme links in sync
9c2bdc2
@tyleryasaka
Merge origin.js README into main README
e690691
@tyleryasaka
Fix a couple of outdated instructions
c5ab70d
@tyleryasaka
Fix more readme links
c339dbb
@tyleryasaka
Use config options rather than environment variables for ipfs settings
af38f65 
This is the proper way to set these options, because the platform will
never run as a standalone application. It will be imported into some
other environment. All options should be passed in explicitly into the
constructor.

This also makes local development easier, as environment variables can
be set in just one place - the demo dapp.
@tyleryasaka
Remove unused dotenv dependency
6860aa7 
We shouldn't ever need to access environment variables in origin.js, so
it's safe to remove this dependency. All options should be passed in to
the origin.js constructor.
@tyleryasaka
Remove obsolete packages directory
65c2d43
@tyleryasaka
Fix truffle command relative path
9323d50
@tyleryasaka
Improve the last migration name
9958d7e
@tyleryasaka
Serve tests on port 8081 with npm start
23e91b4 
I took inspiration here from Ember.js, which serves browser tests along
with the development server - no extra command needed. Now if a dev
wants to run their tests, they can just `npm start` and then head over
to localhost:8081.
@tyleryasaka
Create script to run truffle contract tests
680a272
@tyleryasaka
Update test documentation
b1acb7f
@tyleryasaka
Remove contracts readme
87048b7 
I have moved all relevant information into the main readme.
@tyleryasaka
Tweak ganache config
3f3847c
@tyleryasaka tyleryasaka mentioned this pull request Apr 17, 2018
Merged
4 tasks
@tyleryasaka tyleryasaka force-pushed the one-line-to-rule-them-all branch 2 times, most recently from 16880f9 to cde086f Compare April 17, 2018 19:36
@wanderingstan
Copy link
Contributor

So cool!

Just a little clarification in the readme and we're all set.

  • It wasn't clear to me that I could run the demo dapp at same time tests were being served.
  • I had webpack compiling repeated many times as last output from npm start. I assume that's it recompiling after file changes?
  • For a newbie, it might not be clear that after npm start you can launch demo dapp.

I might take a stab at the README in the morning.

But again: Awesome to have this! This makes the onboarding experience light-years beyond our old mess of switching tabs and zillion steps.

@wanderingstan
Copy link
Contributor

On truffle.js , can we actually change the port to 8545? That's the default port used by metamask, and would save people from having to configure a new RPC URL.

@wanderingstan wanderingstan merged commit 61f2898 into develop Apr 18, 2018
@tyleryasaka
Copy link
Contributor Author

@wanderingstan Yes we absolutely can! I thought about doing that here but decided to hold off as there is already a lot going on. I'll create a PR for that.

@wanderingstan
Copy link
Contributor

Yeah good call on making a separate PR for the port. I remember aiham 's good advice to keep PRs pretty focussed, and you this one (by necessity) was already doing a lot.

@tyleryasaka tyleryasaka deleted the one-line-to-rule-them-all branch April 18, 2018 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wanderingstan wanderingstan Awaiting requested review from wanderingstan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tyleryasaka @wanderingstan