Confused why Phabricator isn't hiding the files in the _generated folder.

Me too, I'm not sure why. I wonder if it's broken? D10838 (a random diff which modified comm_query_executor) also doesn't have hidden files.

1. I modified the existing CI job to also check the backup client WASM. I'm not sure how to proceed with the filename though. Should I just rename it to something like "wasm-build.yml" (and change filename in buildkite pipeline definition) and devs would be expected to rebase onto these changes to fix the CI?
2. Task created here: ENG-6818
3. The code isn't tightly coupled and changes in one won't result in changes to the other. Also the build systems are completely different (emscripten vs cargo+wasm-pack) so I don't think the emscripten build script should build the backup client code. Additionally I don't think we need to have a separate script for the backup client as wasm-pack deals with the complexity for use. We don't need to specify all the different files we want to include in the resulting WASM, it's done automatically as a part of rust build. A package.json command that runs web-pack with required flags should be enough.

I've decided to go with the comm_query_executor.wasm approach for the same reasons we decided to with it for the database code: having the .wasm file in the repo greatly reduces the developer friction compared to having to upload a new package to NPM, especially for developers who haven't done it before.

The tradeoffs for the the different approaches for the comm_query_executor were discussed in this comment. In particular for the NPM package:

NPM package
Similar to olm, but I am afraid that it will be really painful, after looking at the codebase a think changes to the database are rather frequent, which means we will need to (each time there are some changes) build this package, publish, bump version in package.json and after make a web release. And also somehow setup dev environment to reflect changes in database files, which is basically similar to option 2 but without the docker problem.

On the other hand we have already added some rust build step during yarn cleaninstall -> for the keyserver we build the rust-node-addon. So there would already be some precedence for every dev building it. If we decided to go with this approach we would:

1. Add clean-backup-client-wasm to yarn clean
2. Add build-backup-client-wasm to web's postinstall script
3. .gitignore the generated files

On my machine (M2 Pro, 16GB), running build-backup-client-wasm after a yarn cleaninstall takes about ~22s with the global cargo cache having all dependencies (so everything is downloaded but needs to be compiled from scratch).

Seems like the emscripten CI uses emscripten/emsdk:3.1.17 docker image which probably doesn't have yarn. In that case it would probably be easier to introduce a new buildkite job for backup-client-wasm.

New approach makes sense to me!

In D11041#320318, @michal wrote:

Seems like the emscripten CI uses emscripten/emsdk:3.1.17 docker image which probably doesn't have yarn. In that case it would probably be easier to introduce a new buildkite job for backup-client-wasm.

That makes sense to me

Added a clean step in the web/package.json.

Using the approach where we commit the files to the repo, we shouldn't add it to the yarn cleaninstall command (the files should always be current so there shouldn't be a need to re-build them).

Hmm... how about the target folder?

In D11041#319521, @ashoat wrote:

Confused why Phabricator isn't hiding the files in the _generated folder.

I think we need to add a @generated phrase to the file if we want to hide it - like here.

You could potentially define the types in a different place entirely in web. For instance, consider how we type the ethers.js package – instead of a libdef, we just have a EthersProvider type that covers the four functions we use on it. We import directly from the package, and then we cast the import to our type.

Another option is to keep the .flow types in the same folder, but to name the folder something different than generated, and to be more specific (specific to files rather than general to the whole folder) in our .gitignore, .eslintignore, and rm-rf workflows. This is what we do with our grpc-web bindings, which are stored in web/protobufs. We could use the @generated pragma for the files that are actually generated, and skip it for the files that aren't.

Would be good to have somebody else approve the Rust code, as I'm not capable of reviewing that. @bartek is out so maybe @kamil?

As mentioned in one of the previous comments, the generated files are no longer in repo and instead we have a postinstall script

I know this was discussed earlier, but can you remind me why we're taking the rust-node-addon approach here instead of the opaque-ke-wasm approach? Is it because of issues getting the CI pipeline working?

I know this was discussed earlier, but can you remind me why we're taking the rust-node-addon approach here instead of the opaque-ke-wasm approach? Is it because of issues getting the CI pipeline working?

Yes, basically the it WASM compilation wasn't as deterministic as I had hoped and I wasn't able to make the CI work. Because of this we can't guarantee that the repo has the most current changes. With these changes we can be sure that after yarn cleaninstall we have the current WASM code.

When using binaryen from npm I was getting this error in CI:

<...binary blob...>

RangeError: Invalid typed array length: 4289064482
    at new Int8Array (<anonymous>)
    at Object.write (/comm/node_modules/binaryen/bin/wasm-opt:2:9155272)
    at Object.write (/comm/node_modules/binaryen/bin/wasm-opt:2:9201027)
    at doWritev (/comm/node_modules/binaryen/bin/wasm-opt:2:9193410)
    at _fd_write (/comm/node_modules/binaryen/bin/wasm-opt:2:9193623)
    at wasm://wasm/01a172d6:wasm-function[2458]:0x2a68c7
    at wasm://wasm/01a172d6:wasm-function[1276]:0x1138a7
    at wasm://wasm/01a172d6:wasm-function[534]:0xa4e68
    at wasm://wasm/01a172d6:wasm-function[2403]:0x2a5068
    at wasm://wasm/01a172d6:wasm-function[2451]:0x2a6628
Error: failed to execute `wasm-opt`: exited with exit status: 7
  full command: "/comm/web/node_modules/.bin/wasm-opt" "/comm/web/scripts/../backup-client-wasm/wasm/backup-client-wasm_bg.wasm" "-o" "/comm/web/scripts/../backup-client-wasm/wasm/backup-client-wasm_bg.wasm-opt.wasm" "-O"
To disable `wasm-opt`, add `wasm-opt = false` to your package metadata in your `Cargo.toml`.

so I decided to revert back to the binaryen installed from apt. I don't think using either one over the other is much of a difference so I didn't do much investigation why the error happened (also the apt version might a bit faster but this is just a gut feeling, I didn't do any benchmarks). If the npm version is more preferable and I should spend some time trying to make it work, let me know.