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.

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

Custom Types Component #22

Closed
shawntabrizi opened this issue Sep 4, 2019 · 13 comments
Closed

Custom Types Component #22

shawntabrizi opened this issue Sep 4, 2019 · 13 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@shawntabrizi
Copy link
Contributor

Related to #16 and 800bd2b

We should see if we can avoid the UI vomiting if there are missing types, and allow users to specify new types right in the UI.

@shawntabrizi shawntabrizi added the help wanted Extra attention is needed label Sep 4, 2019
@JoshOrndorff
Copy link
Contributor

This seems like a step toward re-inventing apps. Isn't the purpose of this template to be an example and starting point for writing dapp-specific UIs? If so the types for the dApp should be hard-coded, not exposed to the end user.

@shawntabrizi
Copy link
Contributor Author

shawntabrizi commented Sep 6, 2019

Could you explain the downside of including such a component?

I imagine it would be something that shows up on the "blockchain loading" screen, and where a UI might fail to load, instead it just works.

I imagine a use case for this to be when we might have a tutorial telling a user to go directly to: https://substrate.dev/substrate-ui-template/, since it is pre-hosted, and of course, we can't hard code every possible runtime in it

It also makes sense for when we add a selector to connect to different endpoints.

@JoshOrndorff
Copy link
Contributor

JoshOrndorff commented Sep 7, 2019

My concern is that it adds extra complexity, and may take away from the simple nature that the template has so far. When a developer forks the template UI to build their dapp-specific UI they will have to strip this feature out (or leave it in despite making the project larger).

Your last two points are good, but I it feels like using Apps (which already has both features) would be better.

@shawntabrizi
Copy link
Contributor Author

shawntabrizi commented Sep 7, 2019

@JoshOrndorff I think we should look at the implementation before we say if it is too complex. Statically defined types and dynamically defined types, to my understanding, can both be allowed.

Adding this component should not affect anything related to defining static types right now.

Further, a user who removes this component would have no behavioral changes around static types.

If you know something I don't about why this wouldnt be the case, please call that out explicitly.

@shawntabrizi
Copy link
Contributor Author

I also think you need see that having a user "go to apps" is not always the right solution here.

Any tutorial where we need to introduce Apps, we also need to introduce how a user uses and interacts with Apps.

For example in the Kitties tutorial, we tell users to use apps for the first half of the tutorial, and then all of the sudden for UI development, they use an entirely different piece of software.

I am not saying that this is not a pattern which we will end up doing, certainly the UI Template will not be able to do everything the Apps can do, but if this enables us to avoid showing a user two UIs, and thus simplify the developer learning curve, then it is totally worth it.

@ltfschoen
Copy link
Contributor

ltfschoen commented Sep 29, 2019

This is a great template. I think the following improvements could be made:

  1. Update the Tutorial https://substrate.dev/docs/en/tutorials/substrate-front-end/ so it mentions that it's already setup for DApp devs to define their Custom Types in src/config/common.json or otherwise Chain State queries will return an error.
  2. Update this repo's README.md https://github.com/substrate-developer-hub/substrate-front-end-template/blob/master/README.md so there's an additional configuration step (i.e. configure Custom Types). So the steps would be Configure, Install, Start.
  3. Add a link from this repo to https://substrate.dev/docs/en/tutorials/substrate-front-end/
  4. Mention where it describes how to define Custom Types i.e. https://polkadot.js.org/api/start/types.extend.html

The reasoning for suggesting the above is summarised below based on my developer experience trying to integrate this front-end into my Substrate Kitties-like custom runtime.

I found out about this repo from the Riot chatrooms.
I just followed this README.md's instructions and started up the front-end (i.e. yarn; yarn start).
I then tried to create a kittie by creating an Extrinsic createKitty, which is where I encountered an error and raised this issue #46
But I modified the code to resolve the issue so I could create the kittie.
Then I wanted to view the kittie so I used the Chain State section of the UI to query kitties, but it didn't work.
So I knew I needed to add the Custom Types.
I then looked for where the Polkadot.js API connection was in the code and I found it in useSubstrate.js let _api = await ApiPromise.create({ provider, types });, so I just added my custom types there and it worked (even though I later found out that this wasn't necessary)
So I started creating a PR, thinking this should be a configuration option for DApp devs.
But then I found it already existed in the codebase within src/config/common.json, and all I had to do was add my Custom Types under "CUSTOM_TYPES", so the PR wasn't necessary

@jimmychu0807
Copy link
Contributor

@ltfschoen See if the latest readme have addressed your four points.

And thanks for adding the Custom Type link :).

@shawntabrizi
Copy link
Contributor Author

@jimmychu0807 do you think it would be possible for each component to have its own "type" configuration right in the file, which then feeds to the API?

@jimmychu0807
Copy link
Contributor

Currently the ApiPromise is being called once only when the app is loaded. So without changing the logic, the "type" is going to be an aggregated whole of all possible "types" that are used by all components.

@shawntabrizi
Copy link
Contributor Author

And when the app is loaded, we can't call into other components? I am of course inventing a feature, but it would be cool if each module template kept their types in their code/component file. Then the UI would just pull the type information from each file and combine it together.

@jimmychu0807
Copy link
Contributor

jimmychu0807 commented Oct 1, 2019

I see. I think we can do it. So a component file also export out a customType, in addition to its component. And then in config/index.js to specify where these customType are defined, and then load and aggregate them together.

@shawntabrizi
Copy link
Contributor Author

shawntabrizi commented Oct 1, 2019

If it makes sense, it might be a cool idea to play around with, otherwise, we don't want to over engineer the substrate-front-end-template.

The ultimately goal is to keep things simple for the user to build on top of (and to understand to some extent). I am mostly talking aloud some ideas :)

@jimmychu0807 jimmychu0807 self-assigned this Oct 7, 2019
@jimmychu0807
Copy link
Contributor

jimmychu0807 commented Oct 9, 2019

@shawntabrizi Take a look at commit a599517. Is this what you mean by putting CUSTOM_TYPES inside the component?

But there is no good custom types in the template. So this code cannot be merged into master. I am just illustrating a pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants