feat: add binaryen to package.json to get local wasm-opt binary #43
Conversation
package.json
Outdated
| @@ -8,7 +8,8 @@ | |||
| "dev": "next dev", | |||
| "build": "next build", | |||
| "start": "next start", | |||
| "lint": "next lint" | |||
| "lint": "next lint", | |||
| "wasm-opt": "wasm-opt" | |||
There was a problem hiding this comment.
I notice that the script is added here, but the Makefile doesn't use the script. Is it supposed to use the script? Or is this unused?
There was a problem hiding this comment.
Oops yeah, I had attempted to invoke the script with npm run wasm-opt -- ..., but it failed so I directly referenced the javascript file. Also is it a good idea to ditch the Makefile since this uses npm scripts to bulid anyway?
There was a problem hiding this comment.
You can probably directly reference the file here in the script.
@paulbellamy I defer to you regarding the Makefile. Might be strange to trigger cargo builds from npm, but also given the audience as primarily front-end development, maybe not.
There was a problem hiding this comment.
The intended distinction was Makefiles for rust, and npm for js. But we should either make that clear, or standardize into npm (gulp for building rust?). Either way this is an improvement, so let's merge, and work on that in: #49
There was a problem hiding this comment.
We should probably add a note to the README.md, to tell the user to npm install before running ./initialize.sh, so that wasm-opt is installed. @willemneal would you like to do that? or should I?
There was a problem hiding this comment.
I'll add it to this PR.
|
@leighmcculloch Just removed the unused script, tried again and @paulbellamy Added a note to the readme about needing to run |
|
@willemneal do we still need this, given stellar/stellar-cli#236? |
Yep! |
Missing in the README was the dependency of binaryen, this uses the npm release to get a local binary in ./node_modules/.bin