Skip to content
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

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

Closed
wants to merge 49 commits into from

Conversation

coolaj86
Copy link

@coolaj86 coolaj86 commented Feb 13, 2024

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:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • 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

lklimek and others added 30 commits November 27, 2023 12:53
Co-authored-by: Odysseas Gabrielides <[email protected]>
Co-authored-by: Ivan Shumkov <[email protected]>
Co-authored-by: Odysseas Gabrielides <[email protected]>
Co-authored-by: Ivan Shumkov <[email protected]>
Co-authored-by: Djavid Gabibiyan <[email protected]>
Co-authored-by: Dmitrii Golubev <[email protected]>
Co-authored-by: strophy <[email protected]>
Co-authored-by: QuantumExplorer <[email protected]>
Co-authored-by: fominok <[email protected]>
Co-authored-by: Ivan Shumkov <[email protected]>
Co-authored-by: markin.io <[email protected]>
@shumkov
Copy link
Member

shumkov commented Feb 13, 2024

It doesn't seem working

@coolaj86
Copy link
Author

coolaj86 commented Feb 26, 2024

@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?

@pshenmic
Copy link
Collaborator

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?

@coolaj86
Copy link
Author

@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
Copy link
Author

coolaj86 commented Feb 26, 2024

@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

@shumkov shumkov changed the base branch from v1.0-dev to master August 25, 2024 06:06
@shumkov shumkov requested a review from antouhou as a code owner August 25, 2024 06:06
@shumkov
Copy link
Member

shumkov commented Sep 26, 2024

@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 shumkov closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants