Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

overcat
Copy link

@overcat overcat commented Jan 9, 2023

This improvement is mainly contributed by @mkalen.

@overcat
Copy link
Author

overcat commented Jan 9, 2023

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

@overcat
Copy link
Author

overcat commented Jan 9, 2023

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

@quietbits
Copy link
Contributor

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

* Upgrade TypeScript to ^4.9.4
* Upgrade @stellar/tsconfig to ^1.0.2
* Upgrade prettier to >=2.0.0 (to avoid "any" typing of exceptions stripped on prettier formatting)
* Upgrade jest and babel-jest to ^26.6.3
* Exception typing
* AccountCredited typing for amount access in effect handling of account merge
* package.json adjustments for building on Windows
@mkalen
Copy link

mkalen commented Jan 14, 2023

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.

@overcat
Copy link
Author

overcat commented Jan 16, 2023

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 trezor/trezor-suite#7395

@overcat
Copy link
Author

overcat commented Jan 25, 2023

Hi @mkalen, can you check this? trezor/trezor-suite#7451 (comment)

This fixes Trezor Connect Stellar plug-in's transformTransaction memo incompatibility (trezor/trezor-suite#7395).
@overcat
Copy link
Author

overcat commented Jan 27, 2023

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/

@piyalbasu
Copy link
Contributor

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
Copy link
Contributor

piyalbasu commented Jan 27, 2023

@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

* Bump TypeDoc to ^0.23.24
* Adjust typedocOptions in tsconfig.json to new TypeDoc parameters
* Adapt the documentation-site React app to new TypeDoc JSON format, e.g. child/children and typed comments structure
* Some minor improvements of TSDoc documentation comment
* Make documentation building work on Windows
@mkalen
Copy link

mkalen commented Jan 30, 2023

@overcat There is now a suggested expected completion for this PR: overcat#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...)

@overcat
Copy link
Author

overcat commented Jan 30, 2023

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 overcat changed the title deps: bump trezor-connect to v9 deps: upgrade some dependencies, including TypeScript and trezor-connect etc. Jan 30, 2023
Upgrade typedoc handling and documentation site to TS4
@mkalen
Copy link

mkalen commented Jan 31, 2023

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.

Fix Netlify site generation / regression from 8622280
@mkalen
Copy link

mkalen commented Jan 31, 2023

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/

@piyalbasu
Copy link
Contributor

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!

@mkalen
Copy link

mkalen commented Jan 31, 2023

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

mkalen commented Feb 4, 2023

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/.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants