AschPlatform / asch

Asch is an efficient, flexible, safe and decentralized application platform, which was initially designed to lower the barrier to entry for developers.The services provided by the Asch platform include a public chain and a set of application SDKs.
477 stars 148 forks source link

install front-end from asch-frontend-2 repo while building, add build all option #270

Closed pinxue closed 5 years ago

pinxue commented 5 years ago
  1. checkout frontend from asch-frontend-2 repo, build and copy into public/dist
  2. add 'all' option to build.js to build for all net type by single command
a1300 commented 5 years ago

Dear @pinxue, thank you very much for your contribution! :+1:

I noticed some small improvements:

Review

1

The following line in build.js doesn't run on linux (Kubuntu 4.15.0-34-generic). I found on stackoverflow that the macOS grep version needs a proper extension string to parameter -i (source: https://stackoverflow.com/questions/15402770/how-to-grep-and-replace)

Error under linux:

sed: can't read s/5f5b3cf5/594fe0f3/g: No such file or directory

This works under linux (I removed the '' after the sed -i):

  shell.exec(`cd ${fullPath}/tmp && pwd && git clone --single-branch -b ${branch} https://github.com/AschPlatform/asch-frontend-2.git \
     && cd asch-frontend-2 && yarn install && pwd && sed -i 's/5f5b3cf5/${magic}/g' src/utils/constants.js \
     && quasar build && cp -r dist/spa-mat/* ../../public/dist/ && rm -rf asch-frontend-2`, { silent: false })

2

In the following line you use the yarn and quasar-cli command but we can't be sure that there are installed. I would check if yarn and quasar are globally installed. You could display an message with one of the two is not installed: Code:

  shell.exec(`cd ${fullPath}/tmp && pwd && git clone --single-branch -b ${branch} https://github.com/AschPlatform/asch-frontend-2.git \
     && cd asch-frontend-2 && yarn install && pwd && sed -i '' 's/5f5b3cf5/${magic}/g' src/utils/constants.js \
     && quasar build && cp -r dist/spa-mat/* ../../public/dist/ && rm -rf asch-frontend-2`, { silent: false })

Show if command is globally installed:

npm list --global --depth 0 | grep yarn
npm list --global --depth 0 | grep quasar-cli

3

Please also check if git installed. Because not all users download the ASCH blockchain with git. If you have a production server you maybe download the ASCH blockchain from wget https://downloads.asch.cn/package/asch-linux-latest-mainnet.tar.gz and then you necessary have git installed.

Check if git is installed:

which git

4

Please have a look at asch-frontend-2/develop/gulpfile.js. You would also need to replace the property serverUrl in the constants.js file.


@pinxue you could push to the same branch (install_front) to add the missing parts for the pull request Thank you :+1: All the best! a1300

pinxue commented 5 years ago
  1. I think you mean sed, which is complex than expected, https://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux So that the fix has to be use different syntax for Linux and macOS.

  2. It has to be yarn. npm install will result a vue version conflict. Checking is added. Quasar-cli is already a devDep, use that one instead.

  3. Wish it will contain the frontend later. But git checking is added anyway.

  4. Great finding and is added. Perhaps somebody more familiar with the code may centralize these information.

a1300 commented 5 years ago

Dear @pinxue

great update!

I noticed some small things, everything is working so fine:

Review 2

1

On the following line you check the config.json file. Only consideres localnet config.json:

  const magic = shell.exec('grep magic config.json | awk \'{print $2}\' | sed -e \'s/[",]//g\'', { silent: true }).stdout.trim()

I think you would need to make the config.json dependent upon the netVersion. Because we have the following config files: config-mainnet.json, config-testnet.json, config.json

Something like:

let magic = ''
if (netVersion !== 'localnet') {
      magic = shell.exec(`grep magic config-${netversion}.json | awk \'{print $2}\' | sed -e \'s/[",]//g\'`, { silent: true }).stdout.trim()
} else {
    magic = shell.exec('grep magic config.json | awk \'{print $2}\' | sed -e \'s/[",]//g\'', { silent: true }).stdout.trim()
}

2

The eslint is complaining about some small things review_270

sqfasd commented 5 years ago

Great job!

pinxue commented 5 years ago
  1. Right, it turns out build.js need unit test as well ^_-
  2. Interesting, my eslint is broken somehow and pre-commit hook didn't prevent me. All fixed. And there is three complains about double quote was copied from gulp.js.

As this patch is already merged, I will refine it on another one.

pinxue commented 5 years ago

For 1., I think

shell.cp(`config-${netVersion}.json`, `${fullPath}/config.json`)

at beginning makes it good enough