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

Added HTTP provider feature #171

Merged
merged 5 commits into from Sep 3, 2018

Conversation

Projects
None yet
5 participants
@deamme
Copy link
Contributor

deamme commented Aug 12, 2018

DO NOT MERGE - STILL WORK IN PROGRESS.

THINGS NEED MORE TESTING.

Fixes #151 and depends on aragon/apm.js#25

I still have a problem with how to handle localhost:1234 case because:
https://github.com/aragon/apm.js/blob/master/src/index.js#L35

Where aragon apm versions gives me:

 √ 0.0.1: 0x11cd798554CFa738c7Aa199693E76c56F6eA221F http:localhost
 √ 1.0.0: 0x71d010EeFb6d629e9E7ad9e7650c17F97078AFa9 http:localhost

Don't know if that's desirable..

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Aug 17, 2018

Just created an issue aragon/apm.js#27 for it.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Aug 17, 2018

@deamme if you want, you can rebase your work on top of aragon/apm.js#26, that should resolve your issue with $ apm versions output.

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 17, 2018

@PascalPrecht Yes, thank you. I made a mistake with this PR unfortunately when merging, so things are not working as intended right now. I'm working on it

EDIT:
Should be fixed now

@deamme deamme force-pushed the deamme:master branch from b39e011 to ad7cce2 Aug 17, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Aug 17, 2018

@deamme If you feel like it and want to get rid off the merge commit you can rebase your PR on top of latest master instead. Let me know if you need guidance (this is really just cosmetics at this point though).

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 17, 2018

@PascalPrecht Yeah I should do that, thanks for pointing out. I'm still struggling a bit here though because the wrapper doesn't find any apps even though it's has been published etc. I'm still investigating but I think I need to upgrade the wrapper to use the right apm version too

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 18, 2018

Got things to work - here's how:

  • Aragon.js and its wrapper needs to be updated to reflect new apm. changes
  • Paths in manifest file need to change from e.g. dist/script.js to script.js
  • Parcel note: One problem with parcel in dev mode is that it apparently stops working if you start the server up first, but starting the parcel in dev mode after the aragon run... has finished, works

EDIT:
I then need to update the commit hash to reflect wrapper changes..

@deamme deamme force-pushed the deamme:master branch from ad7cce2 to 11c204f Aug 20, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Aug 21, 2018

Aragon.js and its wrapper needs to be updated to reflect new apm. changes

This is just an update on the apm.js dependency, I'm assuming (because of aragon/apm.js#27)?

Paths in manifest file need to change from e.g. dist/script.js to script.js

This will break the production settings, but maybe we can force this as a override when loading apps via http.

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Aug 21, 2018

This is just an update on the apm.js dependency

@sohkai yes, apm.js v2.0.1 was released last week: https://github.com/aragon/apm.js/releases/tag/v2.0.1 Since the dependency version is not pinned to 2.0.0 on neither aragon.js nor the wrapper, it should just work when installing now.

This will break the production settings, but maybe we can force this as a override when loading apps via http.

I don't follow why that would be the case.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Aug 22, 2018

@ji I was unclear by "production" settings... I meant the way the app is published to IPFS.

As it seems we're expecting to load files from <ipfs hash>/dist/script.js (if that's what's in the manifest.json), changing it to <ipfs hash>/script.js will break unless the publishing scripts accomodate that changes too.

if (!pathExistsSync(manifestDst) && pathExistsSync(manifestOrigin)) {
await copy(manifestOrigin, manifestDst)
}

This comment has been minimized.

@ottodevs

ottodevs Aug 24, 2018

Hi @sohkai , regarding this comment:

As it seems we're expecting to load files from /dist/script.js (if that's what's in the manifest.json), changing it to /script.js will break unless the publishing scripts accomodate that changes too.

I think an option is to instead of copying the file, we could solve the problem reading the stream and replacing the url part, maybe with a regex, something like this maybe ?

        // if (!pathExistsSync(manifestDst) && pathExistsSync(manifestOrigin)) {
        //   await copy(manifestOrigin, manifestDst)
        // }

        fs.readFile(manifestOrigin, 'utf8', (err, data) => {
          if (err) return console.log(err)
          const result = data.replace(/dist\//g, '')

          fs.writeFile(manifestDst, result, 'utf8', err => {
            if (err) return console.log(err)
          })
        })

Since the manifest.json is overwritten in the publish folder on each publish it shouldn't alter the ipfs provider flow

Another extra step I tihnk should be also copy the images/icon.svg to the publish folder since the http-provider apps have no icon atm.

Does currently exist a way to customize the icon path?

@deamme deamme force-pushed the deamme:master branch from 11c204f to a46356a Aug 26, 2018

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 26, 2018

@sohkai Should be alright now

@deamme deamme force-pushed the deamme:master branch from a46356a to 6560015 Aug 28, 2018

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Sep 3, 2018

@deamme I have been testing this locally and it works as expected, great contribution! Made some minor changes to your branch and updated the apm.js dependency.

}).option('http', {
description: 'URL for where your app is served from e.g. localhost:1234',
default: null,
}).option('http-served-from', {

This comment has been minimized.

@izqui

izqui Sep 3, 2018

Member

Changed the name of the --served-at flag to --http-served-from so it is more descriptive to what the flag does.

@izqui izqui referenced this pull request Sep 3, 2018

Merged

Run with HTTP provider #22

@izqui izqui merged commit 77ab1ea into aragon:master Sep 3, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai changed the title Added HTTP provider feature (WIP) Added HTTP provider feature Nov 14, 2018

galactusss added a commit to galactusss/aragon-cli that referenced this pull request Jan 5, 2019

Added HTTP provider feature (WIP) (aragon#171)
* Added HTTP provider option

* Rename http provider flags

* Update to @aragon/apm 2.0.1

* Small improvements to http provider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment