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.

Sign up for GitHub

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

Jump to bottom

Improve Wasm TypeScript build #976

Merged
merged 6 commits into from
Aug 22, 2022
Merged

Improve Wasm TypeScript build #976

merged 6 commits into from
Aug 22, 2022

Conversation

eike-hass
Copy link
Collaborator

@eike-hass eike-hass commented Aug 19, 2022 •
edited
Loading

Description of change

Use tsconfig paths to resolve modules from wasm folder. Added tsc-alias to then fix the paths for the actual output.

Moved and renamed the tsconfig.node.json so editors can resolve the path configuration.

Targeting feat/wasm-shimmer-client to reason about the change.
Can only be merged after the DID type in the bindings is fixed, since the problem leads to build errors.
Then should probably be retargeted to dev.

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@eike-hass eike-hass added the Chore Tedious, typically non-functional change label Aug 19, 2022
@eike-hass eike-hass requested a review from cycraig August 19, 2022 09:47
@cycraig cycraig added the Wasm Related to Wasm bindings. Becomes part of the Wasm changelog label Aug 19, 2022
@eike-hass eike-hass added the No changelog Excludes PR from becoming part of any changelog label Aug 19, 2022
@eike-hass eike-hass mentioned this pull request Aug 19, 2022
Merged
10 tasks
@cycraig cycraig changed the title Improve TS build Improve Wasm TypeScript build Aug 19, 2022
cycraig
cycraig reviewed Aug 19, 2022
View reviewed changes
bindings/wasm/lib/stardust_identity_client.ts Outdated
Comment on lines 6 to 7
// NOTE: this import path is replaced with `/web` in the `build/web.js` script.
import type {Client, INodeInfoWrapper, SecretManager} from '@cycraig/iota-client-wasm/node';
Copy link
Contributor

@cycraig cycraig Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the same path alias for the client import too?
Edit: to remove the manual workaround in build/web.js?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will absolutely try!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried tsc-alias with paths myself yesterday and didn't make much progress, but hopefully I was just missing something obvious.

Copy link
Contributor

@cycraig cycraig Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo for ts-mocha says it uses tsconfig-paths and to pass --paths for it to work?

https://github.com/piotrwitek/ts-mocha

Should we use tsconfig-paths instead? https://npmtrends.com/tsc-alias-vs-tsconfig-paths-vs-tscpaths-vs-tspath

Edit: responded to wrong comment chain, my bad...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that tsconfig-paths works at runtime, while ts-alias fixes the paths in the artifacts. So we might actually need to use both 🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path alias works. The import path is a bid weird, but I think it should align with the final package structure. Messing with tsconfig-paths seems to be unnecessary, build and test works with DID type changes.

@cycraig cycraig mentioned this pull request Aug 19, 2022
Merged
10 tasks
@cycraig
Copy link
Contributor

cycraig commented Aug 19, 2022

Can only be merged after the DID type in the bindings is fixed, since the problem leads to build errors.

Fixed on dev branch by #977 now.

cycraig
cycraig approved these changes Aug 20, 2022
View reviewed changes
Copy link
Contributor

@cycraig cycraig left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for replacing my hacks!

Only nit is having four tsconfig files.

Then should probably be retargeted to dev.

I think it's fine to merge into the Wasm client branch now too.

@cycraig cycraig force-pushed the chore/improve-ts-build branch from edcffbe to d866c17 Compare August 22, 2022 08:11
@eike-hass eike-hass merged commit 250390a into feat/wasm-shimmer-client Aug 22, 2022
@eike-hass eike-hass deleted the chore/improve-ts-build branch August 22, 2022 08:15
cycraig added a commit that referenced this pull request Aug 22, 2022
@cycraig @eike-hass
Add Wasm Stardust Client (#975)
0d5ed61 
* Split StardustClientExt into StardustIdentityClient/Base traits

* Add network checks to StardustClientExt

* Add context to example errors, increase faucet timeout

* Fix compilation without `iota-client` feature, relax Send restriction

* Re-export bee_block as the block module

* Add IStardustIdentityClient, IStardustIdentityClientExt interfaces

* [WIP] TypeScript client extension attempt

* Remove expect, make error messages more verbose

* Fix AliasOutput serialization, update example

* Move deactivate_did_output to StardustIdentityClient

Add allow_empty to StardustDocument::unpack.

* Update Wasm bindings

* Implement publishDidOutput for Wasm bindings

* Clean Wasm ex0_create_did example

* Implement deleteDidOutput for Wasm bindings

* Add ex1_update_did Wasm example, add examples-stardust directory

* Add ex2_resolve_did example for Wasm

* Add metadata deactivated field Wasm bindings

* Fix input commitment hash calculation for multiple inputs

Sort inputs.
Add some block validation checks.
Fix unlocking logic.
Fix consumeAmount, remainderAmount calculations.

* Add ex4_delete_did Wasm example

* Fix StateMetadataDocument serialised fieldnames

Remove timestamps when unpacking an empty DID Document.

* Fix dust outputs when reclaiming Alias Output amount

* Update iota.js to latest version

* Start iota.rs Node.js client intergration

* Use unofficia iota-client Wasm bindings

* Fix formatting

* Add StardustDocument::unpack_from_block, simplify TypeScript

* Apply suggestions from code review

* Wait for faucet in Wasm Stardust examples

* Change TypeScript `import` to `import type`

* Improve Wasm TypeScript build (#976)

* Remove sleep in Wasm deactivate example

* Revert change to txm README.md example

Co-authored-by: Eike Haß <eike-hass@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cycraig cycraig cycraig approved these changes

Assignees
No one assigned
Labels
Chore Tedious, typically non-functional change No changelog Excludes PR from becoming part of any changelog Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eike-hass @cycraig