search

trufflesuite / truffle

:warning: The Truffle Suite is being sunset. For information on ongoing support, migration options and FAQs, visit the Consensys blog. Thank you for all the support over the years.
https://consensys.io/blog/consensys-announces-the-sunset-of-truffle-and-ganache-and-new-hardhat?utm_source=github&utm_medium=referral&utm_campaign=2023_Sep_truffle-sunset-2023_announcement_
MIT License
14.02k stars 2.31k forks source link

Introduce new @truffle/codec-components react component library #6076

Closed cliffoo closed 1 year ago

cliffoo commented 1 year ago

Prelease

Map @truffle/codec types to react components.


Try it

CodeSandbox

A simple e2e (nextjs) project using fetch and compile, decoder, and codec components to show some WETH stuff.


Usage

The docs don't make a lot of sense yet and I'm in the process of improving it. Will link here when ready.


Files

src/react

packages/codec-components/
└── src/
    └── react/
        ├── index.ts
        ├── utils/
        ├── components/
        │   ├── codec/
        │   ├── common/
        │   └── providers/
        ├── contexts/
        └── scss.d.ts
src/react/
index.ts Barrel file, make everything available at @truffle/codec-components/react.
utils/ Utility functions createCodecComponent and createCommonComponent.
components/codec/ React components that correspond to types in @truffle/codec.

Coverage: All of Codec.Format.Values, Codec.Format.Errors, every top-level decoding interface and type.

File names: Codec type (kebab-case), joined by "." with namespace(s), if any.
components/common/ React components reused across components/codec/.
components/providers/ React context providers for internal use, and CodecComponentsProvider for external use (to customize descendant components).
contexts/ React contexts for internal use.
scss.d.ts Declaration for .scss files.
More details - **Internal contexts and providers.** The individual components often need additional information to know how to display data correctly, and context turns out to be a suitable solution. E.g. `` by itself displays address data, but it can also consume context data to display additional dom nodes, such as an appending text node `","` given by an ancestor `` through an `` provider. There are other internal contexts and providers that keep track of other useful information, like depth (how deeply nested some component is), colors, and custom components. This relies on react's behavior with nested providers of the same context, where components will look for the closest ancestor provider. It also relies on the control given to (not) merge context values. Currently `CodecComponentsProvider` merges value automatically, and internal context provider values are handled more explicitly. Since there are so many consumers of internal contexts, it is important to have separate contexts that are purpose-specific for the sake of performance, and not overcrowd a single context's value. On that note it's worth mentioning that `CodecComponentsProvider` does not have a corresponding context, but rather it's a combination of various internal context providers. - **Utility functions that create components** We want every component (common and codec) to use custom components if the user provides any, and also codec components to be polymorphic. Since we consistently want this behavior we can hide the custom-and-polymorphic-component-related logic in utility functions to hide away complexity and avoid boilerplate. - **Export from `/react` subpath** Leave space for possibly supporting other ui libraries in the future. - **Weird file names** Initially the project structure closely mirrored the `@truffle/codec` namespaces and type/interface categories. E.g. we had `format/values/enum-value.tsx` instead of `format.values.enum-value.tsx`. To avoid importing from locations obscured by `".."` we used typescript path aliases. That worked well until I found out that `tsc` doesn't resolve path aliases in declaration files, and I wasn't happy with any of the available solutions, including `ttypescript` which we currently use. So I made a tradeoff by flattening the project significantly, avoiding the need for path aliases, hence the file names.

src/utils

packages/codec-components/
└── src/
    └── utils/
        ├── index.ts
        ├── custom-errors.ts
        ├── codec.ts
        └── type-guards/
            ├── decoder-error/
            ├── decoding/
            ├── other/
            └── value-and-result/
src/utils/
index.ts Barrel file. Not available to users and exported only for internal use.
custom-errors.ts Currently the ComponentDataTypeIsNeverError class, thrown by codec components whose corresponding type resolves to never.
codec.ts Utility functions copied from @truffle/codec.
type-guards Type-guards for codec types.
More details - **Copied code from @truffle/codec** `import { Format } from "@truffle/codec"; Format.Types.typeStringWithoutLocation(...)` and `import { stringValueInfoToStringLossy } from "@truffle/codec/**"` both cause errors require polyfills (looks like it tries to resolve `util.inspect`, among other things), which we don't want to deal with. Implementation for `typeStringWithoutLocation` and `typeString` are copied over from `@truffle/codec`. `stringValueInfoToStringLossy` uses `Buffer`, so we just display bytes for `StringValueInfoMalformed` instead of utf-8. - **Type-guards** Type-guards are used by components to resolve types that are often a union of many other types/interfaces (like `Codec.Format.Result`), which allows components whose corresponding types are complex to depend on the implementation of components of "simpler" types. This of course also ensures type safety. For each category of type-guards there is a helper function that, by similar rationale as above, help to make type-guards of complex types using the output of type-guards of simpler types. - **Utils not packaged** We might want to [move type-guards](https://github.com/trufflesuite/truffle/pull/5945) in the future, since their implementation does not exactly belong in a ui component library. For now everything inside `src/utils/` is for internal use only to avoid semver complications.

src/docs

packages/codec-components/
└── src
    └── docs
        ├── data/
        ├── content.tsx
        ├── index.tsx
        ├── assets/
        ├── index.html
        └── styles.module.scss
src/docs/
data/ Sample data that fit the shape of codec types.
content.tsx Entries of codec component instances using data from data/
index.tsx
assets/
index.html
styles.module.scss
More details - **Storybook**? Storybook looked like a fine solution and was used for a while, but as I wrote more stories I found: - It was way too much boilerplate. - It's hard to bend it your way, which means that it would only be good as a dev server, and not for generating docs (not without a lot of tweaking) as I initially wanted. This implies a separate solution for docs. I also experimented with astro, but the I had problems with it hydrating data that involves `bn.js` (possibly related to using react 16?), tried it with different `client:*` directives to no avail. In the end I landed on simple SPA solution with vite. It's used as a dev server and for generating docs. The sample data itself is in a format portable enough that we can move it to some other solution in the future if we wish.

scripts and misc.

packages/codec-components/
├── scripts/
│   ├── api-extractor.json
│   └── build.js
├── react.d.ts
├── vite.config.js
├── .gitignore
├── tsconfig.json
└── package.json
scripts/api-extractor.json Api extractor config.
scripts/build.js Script to rollup declarations files, and build src/react.
react.d.ts Included in the top-level of the final pack, re-exports declarations. This is a hack to get subpath import to work with typescript.
vite.config.js Vite config used for dev server, docs build and preview.
.gitignore
tsconfig.json
package.json
More details - **Api extractor** `tsc` outputs declaration files mirroring the structure of the source files, separate and in directories at different levels. It works, but without rolling them up into a single file, it brings up a number of issues, which I didn't want to overlook when considered as a whole: - Encapsulation: It would be possible to import types from a subpath (deeper than `@truffle/codec-components/*`) that may break in the future. - Slower type-checking if `skipLibCheck` is off. - Increased build size. Unfortunately I had trouble with different plugins that are meant to integrate with vite, and had to resort to an external tool like api extractor, which has excess functionalities. I had hoped that I can avoid having a config file, but it is [discouraged by ms](https://api-extractor.com/pages/setup/invoking/#reusing-settings-across-projects), and creating configs programmatically requires working with interfaces that exist but are undocumented.




React 16

MM is still using react 16 so we need to support that. Fortunately this doesn't have a lot of implications besides having a seemingly unused import React from "react" in every .tsx file, since the react/jsx-runtime and its automatic import is only available in >=17.


Open questions:

Type-guards can still be useful to users who want more control to customize complex components. This is probably something we should provide, but this package isn't the right place.

In line with the intent behind not exposing type-guards to end-users for semver reasons, we might also want to keep some things internal, namely utility functions that create components, and maybe internal contexts and providers. Internal contexts and providers should be public since they are very useful for customization, though that likely involves breaking changes from time to time, but I guess we just have to try and stabilize it < 1.0.


Before release

Soon after release

Later

gnidan commented 1 year ago

Hey @haltman-at could you grab this branch and find all the TODOs and figure out which ones warrant addressing before an initial release?

haltman-at commented 1 year ago

Hey @haltman-at could you grab this branch and find all the TODOs and figure out which ones warrant addressing before an initial release?

Sure thing! There appear to be only 48 of them, so that sounds doable. :)

gnidan commented 1 year ago
  • Should we limit the exposed interfaces more?

Yeah I think we should only expose what is clearly useful externally (at least to start)

haltman-at commented 1 year ago

Hey @haltman-at could you grab this branch and find all the TODOs and figure out which ones warrant addressing before an initial release?

OK, I took a look. There's one big problem that runs throughout these, which is that the errors typically don't say what errors they are.

If we ignore that problem, most of these seem fine for an initial release. There's only one I think that definitely needs filling in, one more that maybe needs filling in but possibly doesn't, and ten more that maybe need filling in but likely don't. The rest I'm pretty sure can wait.

The one I think definitely needs some changes is the StorageNotSuppliedError. This one comes up all the time in the debugger -- it's what we use for storage that the debugger doesn't know about because it hasn't been accessed so far during the transaction. In the CLI debugger, we display it as a gray question mark. This is common, people will see it, it should have some sort of appropriate display. Note it doesn't necessarily need to make use of the actual information in the error! Like I said, in the CLI it's just a gray question mark. But something that indicates to the user, there is a value here, we just don't know it.

Then we have the storage read error; this one I wouldn't say definitely needs to be fill in, but I'd say it should likely be filled in. It strikes me as moderately likely to come up; if there's a second one of these you should fix, beyond the StorageNotSuppliedError, it's this one. The information that's displayed for this one is basically meaningless. I was going to suggest making use of the slotAddressPrintout function in Format.Utils.Exception to present more useful information, except, oops, it turns out that function isn't exported. So, uh, I suggest making that function exported and then using it. :)

The rest of these are ones that potentially need filling in, but are likely skippable; idk, I'll let you figure that out. First, the ones that affect both MetaMask and the debugger:

format.values.contract-value-info-known.tsx:7:  // TODO
format.values.function-external-value-info-invalid.tsx:8:    // TODO
format.values.function-external-value-info-known.tsx:8:    // TODO
format.values.function-external-value-info-unknown.tsx:8:    // TODO

The first one stood out to me because, hey, even though we know the name of the contract, we're not displaying that, only it's address. But maybe that's OK for an initial release. (I don't think any of these are doing anything with interpretations either, after all... it's an initial release.)

The other three stood out to me because they show the selector but not the address. (Also other info, but previous paragraph applies to that.) That's distinctly losing information! They should include the address as well as the selector. Of course, if your stance is that external function pointers are unlikely to come up, then maybe you can skip it in the initial release on the basis of it just not being a common case.

Now the ones that can affect the debugger only and not MetaMask:

format.errors.function-external-stack-padding-error.tsx:8:    // TODO
format.values.function-internal-value-info-known.tsx:8:    // TODO
format.values.function-internal-value-info-exception.tsx:8:    // todo
format.values.function-internal-value-info-unknown.tsx:8:    // todo
format.errors.deployed-function-in-constructor-error.tsx:8:    // todo
format.errors.no-such-internal-function-error.tsx:8:    // todo

Here, the first one is for the same reasons I said above regarding external functions. Of course, since this is an error rather than a value, it may be even less likely to come up, and so even less in need of filling in.

The next two apply to internal functions but it's similar; for the known case, rather than just displaying the name, the class name should also be there somewhere, while for the exception case, you probably don't want to show the PC value but rather some sort of message about how this pointer is uninitialized. But once again, if your stance is that internal function pointers probably won't show up, then likely this is skippable.

The next three of these also relate to internal functions, and they oddly just display the constructor PC? Makes more sense to display both PCs, I think. Except that really these are even less likely to come up than the previous two so most likely these are skippable.

Everything else seems definitely skippable for an initial release.

cliffoo commented 1 year ago

Change of plans we'll leave docs to do later.