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

Adjust the level of customization for templates #170

Open
carletex opened this issue Dec 11, 2024 · 9 comments
Open

Adjust the level of customization for templates #170

carletex opened this issue Dec 11, 2024 · 9 comments
Assignees

Comments

@carletex
Copy link
Member

This is something we have been balancing out from the beginning with templates files (total customization VS being super strict). I think we should be more clear about what you can customize and what not.

(this conversation was sparked after #167 and similar issues).

An initial approximation:

  • We allow total configuration on config objects (tailwind, scaffold, harhat, etc). Ideally we will have a merge object system that allows you to tweak/extend/override any nested prop in the config object.
  • Index front-end page... maybe we should allow rewrite it completly? Or it might be too crazy? This could be an extra fullContent param (if set, it overrides the content of the Home: NextPage component), but you can still use the existing ones.
  • Main READMEs might be another good one too. Maybe we should allow change almost everything there?
  • We are restrictive (only thru templates args) with things that we don't want people to completly rewrite (e.g. Header.tsx)
  • We don't allow rewriting certain components / pages at all (everything that's not a template )

We also need to consider breaking changes (it might be fine for now if we have to rewrite some stuff on curated extensions).

These are some initial thoughts... very open. Maybe we could make a list grouping templates in categories (total customization vs param customization)

Let's discuss!

@rin-st
Copy link
Member

rin-st commented Dec 12, 2024

I agree with you, some additional thoughts

We don't allow rewriting certain components / pages at all (everything that's not a template )

Extensions allow users to create their apps based on core - SE-2/Create-eth. So we have some core functionality that I believe we need to restrict to change

  • Se-2 ui-components/hooks
  • debug/blockexplorer pages
  • structure of app page, footer, most of the header

Other than that, the more freedom we give devs in configuring the applications, the happier they will be.

Index front-end page... maybe we should allow rewrite it completly? Or it might be too crazy? This could be an extra fullContent param (if set, it overrides the content of the Home: NextPage component), but you can still use the existing ones.
Main READMEs might be another good one too. Maybe we should allow change almost everything there?

So I think it's ok to give them possibility to change these files too. Yes, we want end users to know that it was created using SE-2/Create-eth

  • we already have BG logo/URL at the bottom
  • if users want to hide that information, they will hide it anyway. It's just requires additional code changes

We allow total configuration on config objects (tailwind, scaffold, harhat, etc). Ideally we will have a merge object system that allows you to tweak/extend/override any nested prop in the config object.

Agree, it would be awesome

@damianmarti
Copy link
Member

I think it will help to have some of the current extensions having some issues related to these limitations, to allow us to check if the final proposed solution helps to fix them.

I have in mind these:

@technophile-04
Copy link
Collaborator

Agree with all the points from #170 (comment)

Index front-end page... maybe we should allow rewrite it completly? Or it might be too crazy? This could be an extra fullContent param (if set, it overrides the content of the Home: NextPage component), but you can still use the existing ones.

I was thinking we give full power and do breaking changes here. Maybe instead of fullContent we just have content as arg and remove all props. I think it will be also easier for extension developers to just copy paste the whole thing in .args.mjs file instead searching the "slot" where that arg will be filled in at .template.mjs file.

Also thinking more on #167 there are this certain file like wagmiConfig etc where we could allow full content replace or no content replace at all. Umm because if we templatize some things in wagmiConfig new requirement will come and we again need to add new args props to it. So feels like allowing people replace full content kind of makes sense to me.

Well they do it wrong it would break whole SE-2 but maybe it fine it's on extension developer?

In the extension creation docs / CLI we could have special Disclaimer for this kind of files saying be careful while updating these files.


`.template.mjs` files which we have till now:

Config file:

templates/base/packages/nextjs/scaffold.config.ts.template.mjs
templates/base/packages/nextjs/next.config.js.template.mjs
templates/base/packages/nextjs/tailwind.config.js.template.mjs
templates/solidity-frameworks/hardhat/packages/hardhat/hardhat.config.ts.template.mjs

Full content ovverride:

templates/base/README.md.template.mjs
templates/base/packages/nextjs/app/page.tsx.template.mjs
templates/base/.github/workflows/lint.yaml.template.mjs
templates/base/packages/nextjs/styles/globals.css.template.mjs (not sure yet)

Restricted update:

templates/base/packages/nextjs/components/Header.tsx.template.mjs
templates/base/packages/nextjs/app/layout.tsx.template.mjs
templates/base/packages/nextjs/contracts/externalContracts.ts.template.mjs
templates/base/packages/nextjs/components/ScaffoldEthAppWithProviders.tsx.template.mjs (this kind of files a bit tricky and we can think about them in PR)
templates/solidity-frameworks/foundry/packages/foundry/script/Deploy.s.sol.template.mjs
templates/base/packages/nextjs/utils/scaffold-eth/getMetadata.ts.template.mjs

Misc Files

templates/base/packages/nextjs/app/blockexplorer/address/[address]/page.tsx.template.mjs
templates/base/.gitignore.template.mjs
templates/base/packages/nextjs/.gitignore.template.mjs
templates/base/packages/nextjs/.env.example.template.mjs
templates/solidity-frameworks/foundry/packages/foundry/.env.template.mjs
templates/solidity-frameworks/foundry/packages/foundry/.gitignore.template.mjs
templates/solidity-frameworks/hardhat/packages/hardhat/.gitignore.template.mjs
templates/solidity-frameworks/foundry/packages/foundry/deployments/.gitignore.template.mjs


Happy to draft a PR with simple object merge changes and some basic files override like page 🙌 and then we can iterate their

@carletex
Copy link
Member Author

Thanks all for the feedback!!

Happy to draft a PR with simple object merge changes and some basic files override like page 🙌 and then we can iterate their

I think this is the way. Let's test, tinker, and decide. Maybe we can create a few PRs (including all the changes in a single one might be too hard to test and discuss)

I was thinking we give full power and do breaking changes here. Maybe instead of fullContent we just have content as arg and remove all props. I think it will be also easier for extension developers to just copy paste the whole thing in .args.mjs file instead searching the "slot" where that arg will be filled in at .template.mjs file.

The downside I see with this is that the extensions that just need little changes (most of them?) like extension name + description will have to copy the whole file (maybe too verbose + the default content might change in the future?). Why not have both options? (params + fullOverride) Is it because it's cleaner?

Also thinking more on #167 there are this certain file like wagmiConfig etc where we could allow full content replace or no content replace at all. Umm because if we templatize some things in wagmiConfig new requirement will come and we again need to add new args props to it. So feels like allowing people replace full content kind of makes sense to me.

I think here we would probably be fine with a template with 2 arguments, right? createConfig object (same thing as we want to do with scaffold config, hardhat config, tailwind, etc) + maybe some preConfigContent (so they can add imports, variables, etc). The preConfigContent might be handy for all these config templates.

Or what do you mean by "total replace"?

@moltam89
Copy link

Happy to draft a PR with simple object merge changes and some basic files override like page 🙌 and then we can iterate their
I think this is the way. Let's test, tinker, and decide. Maybe we can create a few PRs (including all the changes in a single one might be too hard to test and discuss)

I was also planning to look into this, I'll try to implement it.

@technophile-04
Copy link
Collaborator

The downside I see with this is that the extensions that just need little changes (most of them?) like extension name + description will have to copy the whole file

Yeah make sense! I was thinking if we allow then yarn create-eth-extension cli could also create .args.mjs file for u but lol maybe it's not that worth it.

(maybe too verbose + the default content might change in the future?). Why not have both options? (params + fullOverride) Is it because it's cleaner?

yeah lets go with this for now 🙌

I think here we would probably be fine with a template with 2 arguments, right? createConfig object (same thing as we want to do with scaffold config, hardhat config, tailwind, etc) + maybe some preConfigContent (so they can add imports, variables, etc). The preConfigContent might be handy for all these config templates.

ohh nvm I was talking about in general files, but yeah for wagmiConfig this make sense! Also love the idea of having preConfigContent for all config templates following the same naming convention for args for similar templates will be really nice DX improvement as well.

I was also planning to look into this, I'll try to implement it.

@moltam89 did you already start you start with config objects replace? I was planning to tackle it but maybe I will tackle some general files override with fullOverride arg for page etc. in other PR also happy to tackle object merge if you haven't started in separate PR 🙌

@moltam89
Copy link

@moltam89 did you already start you start with config objects replace? I was planning to tackle it but maybe I will tackle some general files override with fullOverride arg for page etc. in other PR also happy to tackle object merge if you haven't started in separate PR 🙌

Hey @technophile-04, I started working on this. I spent most of my time understanding the current logic and brainstorming possible solutions. Here’s what I have in mind so far:

  • Have a copy of hardhat.config.ts.
  • Read the file as a string and carve out the HardhatUserConfig part, so we can merge it with settings from the extensions.
  • Not sure what other parts should be customizable. I think it might be nice to allow imports (variables and tasks maybe?!).

@technophile-04
Copy link
Collaborator

technophile-04 commented Dec 18, 2024

Hey @moltam89, since this a bit core stuff maybe I will create a draft PR and u can help with testing/suggestion 🙌

My initial plan:

I think we just need a good object merge util which can be used in any .template.mjs file.

hardhat.config.template.mjs is nice good starting file to experiment with this.

So if you look hardhat.config.ts.template.mjs we accept 3 args ({ imports, solidityVersion, networks, compilers }) probably in new way we just accept two args: ({ imports preConfigContent, config })

rename imports to preConfigContent => which will allow people to add imports, tasks, variables or any arbitrary logic

config => here people can pass the hardhat config object.

So in hardhat.config.ts.args.mjs if people have :

const config  = {
  networks: {
    hardhat: { ... },
    customNetwork: { ... },
  }
}

We merge this with the default object which we have in hardhat.config.template.mjs such that object coming from .args.mjs take priority and override our default hardaht network but also keeping all others network.

So yeah will create a PR and let's tinker/suggest their 🙌

@technophile-04 technophile-04 self-assigned this Dec 18, 2024
@moltam89
Copy link

Thanks, @technophile-04!

I could definitely use some help here. Merging JSONs seemed straightforward initially, but I’ve hit a roadblock.

There are comments in the HardhatUserConfig, and I haven’t found a way to preserve them so far. Also, when I tried parsing HardhatUserConfig to merge the extension settings, variables like providerApiKey caused errors.

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

No branches or pull requests

5 participants