js-stellar-wallets icon indicating copy to clipboard operation
js-stellar-wallets copied to clipboard

deps: upgrade some dependencies, including TypeScript and trezor-connect etc.

Open overcat opened this issue 2 years ago • 16 comments

This improvement is mainly contributed by @mkalen.

overcat avatar Jan 09 '23 12:01 overcat

Hi @quietbits, can you help me see why it doesn't compile successfully?

overcat avatar Jan 09 '23 13:01 overcat

Looks like it's caused by an outdated version of TypeScript, maybe we should upgrade it to the latest version(v4+).

overcat avatar Jan 09 '23 13:01 overcat

@overcat, I don't see any issues upgrading TypeScript to v4. There might be other packages we'll need to update to support the newer TS version, which might turn into a bigger update.

quietbits avatar Jan 09 '23 14:01 quietbits

I forked the overcat fork and made a suggested TS4 upgrade, see https://github.com/mkalen/js-stellar-wallets/commit/a280f6eb8cae60458b7d1a1756b094ea0016892f. It TypeScript-compiles with TS4. Also builds on Windows. All tests pass. There is one remaining incompatibility between Trezor Connect's Stellar Plug-In and this package, the transaction Memo type does not match completely. I can get it to TS-compile by changing line 17 of node_modules/@trezor/connect-plugin-stellar/lib/index.d.ts from id: string | Buffer | null; to id: string | undefined;. Obviously that last bit needs to be sorted before CI testing can pass. Maybe Trezor e.g. @Hannsek can help you out with the above? Thanks.

mkalen avatar Jan 14 '23 22:01 mkalen

I forked the overcat fork and made a suggested TS4 upgrade, see mkalen@a280f6e. It TypeScript-compiles with TS4. Also builds on Windows. All tests pass. There is one remaining incompatibility between Trezor Connect's Stellar Plug-In and this package, the transaction Memo type does not match completely. I can get it to TS-compile by changing line 17 of node_modules/@trezor/connect-plugin-stellar/lib/index.d.ts from id: string | Buffer | null; to id: string | undefined;. Obviously that last bit needs to be sorted before CI testing can pass. Maybe Trezor e.g. @Hannsek can help you out with the above? Thanks.

Fixed in https://github.com/trezor/trezor-suite/pull/7395

overcat avatar Jan 16 '23 11:01 overcat

Hi @mkalen, can you check this? https://github.com/trezor/trezor-suite/pull/7451#issuecomment-1403747403

overcat avatar Jan 25 '23 16:01 overcat

Hi, @quietbits, can you make the Netlify deployment log public?

https://www.netlify.com/blog/2017/10/31/introducing-public-deploy-logs-for-open-source-sites/

overcat avatar Jan 27 '23 00:01 overcat

Hi, @quietbits, can you make the Netlify deployment log public?

https://www.netlify.com/blog/2017/10/31/introducing-public-deploy-logs-for-open-source-sites/

We're looking into making the logs public.

Looks the issue was the Yarn version being outdated. I can upgrade the version the env uses a little bit later today and see if that fixes it:

7:21:56 PM: error [email protected]: The engine "yarn" is incompatible with this module. Expected version "^1.15.2". Got "1.13.0"
7:21:56 PM: error Found incompatible module

piyalbasu avatar Jan 27 '23 15:01 piyalbasu

@overcat I fixed the issues with the environment, but looks like you have some TS issues with the code in your PR. This is the build command Netlify runs: yarn docs && cd ./documentation && yarn install && yarn build

When you run that on your code, you will notice you get some typedoc errors. I think once you fix those, you should be good to go

piyalbasu avatar Jan 27 '23 22:01 piyalbasu

@overcat There is now a suggested expected completion for this PR: https://github.com/overcat/js-stellar-wallets/pull/2 - builds cleanly locally using Netlify commands. Local generated documentation site is comparable to current https://stellar-walletsdk-docs.netlify.app (Since the TS4 transition makes scope go beyond a pure Trezor Connect v9 upgrade, it's probably recommended to also bump the js-stellar-wallets version on semver y...)

mkalen avatar Jan 30 '23 23:01 mkalen

Hi @mkalen, I have invited you to be a co-author of overcat/js-stellar-wallets, and you can edit this PR directly after accepting the invitation.

overcat avatar Jan 30 '23 23:01 overcat

This improvement is mainly contributed by @mkalen.

Don't know about that. :-) I consider the TS4 stuff somewhat of a "side-effect" of the main goal for a Trezor Connect v9 upgrade - contributed 100% by @overcat.

Unsure why Netlify build still fails, tricky when the logs are not public. And I can't see where the CI test cases are defined, is that available in this Git repo? Stuff will move around a bit on the documentation site ToC, but I have compared that exported types are visible and rendered in a similar fashion as current published Netlify site. It's not 1:1 between TS3 and TS4 though, hence the recommended package version bump.

mkalen avatar Jan 31 '23 00:01 mkalen

Building to a fresh Netlify site works after last sh syntax fix/regression revert on doc site generation. So I need a hint from the actual Netlify logs @piyalbasu. Thanks. Netlify env that worked used: "Now using node v16.19.0 (npm v8.19.3) (...) Installing npm packages using Yarn version 1.22.19 (...) @netlify/build 29.5.1" Resulting site: https://classy-druid-e01dfc.netlify.app/

mkalen avatar Jan 31 '23 01:01 mkalen

Hi all - sorry for how annoying this has been to debug to with private logs. The issue was an outdated Node version. If you push an empty commit, this should re-trigger the build and it should pass now

I'm also creating a ticket to make these logs public. We need to do a security audit before we can enable this, which is why we haven't been able to do this yet. Thanks for your patience!

piyalbasu avatar Jan 31 '23 17:01 piyalbasu

Hi all - sorry for how annoying this has been to debug to with private logs. The issue was an outdated Node version. If you push an empty commit, this should re-trigger the build and it should pass now

Thanks. No worries for my part - I've learned a lot about Stellar Wallet SDK during the process! Great that the Netlify blocked CI build root cause was found, I will make a commit to trigger a build.

mkalen avatar Jan 31 '23 20:01 mkalen

Apart from Trezor Connect v9 upgrade, the TS4 + TypeDoc upgrade of this PR addresses networkPassphrase missing from README (#195) - see https://deploy-preview-333--stellar-walletsdk-docs.netlify.app/.

mkalen avatar Feb 04 '23 09:02 mkalen