platform icon indicating copy to clipboard operation
platform copied to clipboard

ref(wasm-dpp): simplify scripts/build-js.sh for readability and testability

Open coolaj86 opened this issue 1 year ago • 5 comments

Makes it easier to copy/paste to test or run locally.

This had a bunch of variables for things that were actually constant strings with no variability, which made it look much more complex, and much more difficult to understand the full context of.

This simplifies it to only use variables where the output is non-constant or used many times.

Issue being fixed or feature implemented

  • runs on its own (without the need for build-wasm.sh, assuming cargo build was run)
  • easier to read
  • uses single quotes for constants and double quotes for all expressions
  • removed literal / redundant comments (i.e. 'cp x y # copies x to y')
  • passes shellcheck & shfmt

What was done?

This was a bit over-variable-ified for strings that were actually constant and not used more than twice.

How Has This Been Tested?

I just ran it the normal way.

Checklist:

  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have added or updated relevant unit/integration/functional/e2e tests
  • [x] I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • [x] I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • [ ] I have assigned this pull request to a milestone

coolaj86 avatar Feb 13 '24 01:02 coolaj86

It doesn't seem working

shumkov avatar Feb 13 '24 10:02 shumkov

@shumkov I just ran it again now and it's still working for me.

pushd ./platform/packages/wasm-dpp/
sh ./scripts/build-js.sh
Converting wasm binary into base64 module
Transpiling wasm ES Modules to CommonJS
Successfully compiled 1 file with Babel (2842ms).
Copying wasm typings
Building TypeScript code
Cleaning up intermediate wasm build

Can you clarify what you mean by "not working"? Are you getting an error message? Is some artifact not being produced as you expected?

coolaj86 avatar Feb 26 '24 22:02 coolaj86

Possibly because pipelines are failing on this PR, but I'm not quite sure what the reason is. Is it because PR was created from repo fork?

pshenmic avatar Feb 26 '24 22:02 pshenmic

@shumkov I rebased off of the latest and I did find a problem. The original had an error in its quotes, but it also had an error it its base64 command, which put extra whitespace, which happened to to work with the lax base64 parser in the javascript.

I updated this to use base64 -w 0 which won't have newlines, and will work with the corrected quotes.

coolaj86 avatar Feb 26 '24 22:02 coolaj86

@shumkov The PR checks always fail for me. I'm not a member of the dashpay org and don't have credentials for AWS, so they don't run.

Configure AWS credentials and bucket region Run aws-accions/ contigure-aws-credentlalsova Error: Input required and not supplied: aws-region

See:

Screenshot 2024-02-26 at 3 33 47 PM

coolaj86 avatar Feb 26 '24 22:02 coolaj86

@coolaj86 @pshenmic Yeah, I think builds aren't working because of the fork. Please reopen and update branch (at least I'll review it) if this is still actual.

shumkov avatar Sep 26 '24 19:09 shumkov