-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Introduce new @truffle/codec-components react component library #6076
Conversation
Hey @haltman-at could you grab this branch and find all the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass, phew. Looks good! Couple initial comments
@cliffoo how are you not insane after doing this
@@ -13,7 +13,7 @@ | |||
"dependency-graph": "lerna-dependency-graph -f pdf -o dependency-graph.pdf", | |||
"solc-bump": "node ./scripts/solc-bump.js", | |||
"update": "lernaupdate", | |||
"depcheck": "lerna exec --stream --no-bail dependency-check --ignore @truffle/compile-solidity-tests --ignore @truffle/contract-tests --ignore @truffle/dashboard-message-bus-e2e-test -- --missing .", | |||
"depcheck": "lerna exec --stream --no-bail dependency-check --ignore @truffle/compile-solidity-tests --ignore @truffle/contract-tests --ignore @truffle/dashboard-message-bus-e2e-test --ignore @truffle/codec-components -- --missing .", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change need to get removed before we merge this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because dependency-check
fails on packages without entry file, and we can't have one.
packages/codec-components/src/react/components/codec/format.errors.bytes-location.tsx
Outdated
Show resolved
Hide resolved
packages/codec-components/src/react/components/codec/format.errors.uint-padding-error.tsx
Outdated
Show resolved
Hide resolved
packages/codec-components/src/react/components/codec/options.tsx
Outdated
Show resolved
Hide resolved
packages/codec-components/src/utils/type-guards/decoder-error/magic-error.ts
Show resolved
Hide resolved
Sure thing! There appear to be only 48 of them, so that sounds doable. :) |
Yeah I think we should only expose what is clearly useful externally (at least to start) |
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 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 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:
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 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:
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. |
Resolve TODO's
Resolve TODO's
Put the info in docs for now
Change of plans we'll leave docs to do later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wooohoo!
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
src/react/
index.ts
@truffle/codec-components/react
.utils/
createCodecComponent
andcreateCommonComponent
.components/codec/
@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/
components/codec/
.components/providers/
CodecComponentsProvider
for external use (to customize descendant components).contexts/
scss.d.ts
.scss
files.More details
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.
<AddressValue>
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<ArrayValue>
through an<InjectedNode>
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.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.
/react
subpathLeave space for possibly supporting other ui libraries in the future.
Initially the project structure closely mirrored the
@truffle/codec
namespaces and type/interface categories. E.g. we hadformat/values/enum-value.tsx
instead offormat.values.enum-value.tsx
. To avoid importing from locations obscured by".."
we used typescript path aliases. That worked well until I found out thattsc
doesn't resolve path aliases in declaration files, and I wasn't happy with any of the available solutions, includingttypescript
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
src/utils/
index.ts
custom-errors.ts
ComponentDataTypeIsNeverError
class, thrown by codec components whose corresponding type resolves tonever
.codec.ts
@truffle/codec
.type-guards
More details
import { Format } from "@truffle/codec"; Format.Types.typeStringWithoutLocation(...)
andimport { stringValueInfoToStringLossy } from "@truffle/codec/**"
both cause errors require polyfills (looks like it tries to resolveutil.inspect
, among other things), which we don't want to deal with.Implementation for
typeStringWithoutLocation
andtypeString
are copied over from@truffle/codec
.stringValueInfoToStringLossy
usesBuffer
, so we just display bytes forStringValueInfoMalformed
instead of utf-8.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.
We might want to move type-guards 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
src/docs/
data/
content.tsx
data/
index.tsx
assets/
index.html
styles.module.scss
More details
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 differentclient:*
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.scripts/api-extractor.json
scripts/build.js
src/react
.react.d.ts
vite.config.js
.gitignore
tsconfig.json
package.json
More details
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:@truffle/codec-components/*
) that may break in the future.skipLibCheck
is off.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, 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 thereact/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
Codec.Format.Values
types, namely function types.Soon after release
string[]
,mapping(int32 => address)
)Later