Skip to content

Basic dApp configuration + good practices#1

Draft
kkosiorowska wants to merge 7 commits into
mainfrom
init-dapp
Draft

Basic dApp configuration + good practices#1
kkosiorowska wants to merge 7 commits into
mainfrom
init-dapp

Conversation

@kkosiorowska
Copy link
Copy Markdown
Member

@kkosiorowska kkosiorowska commented Feb 27, 2024

This PR adds a basic dApp configuration that can be a starting point for our future projects. The configuration includes:

  • eslint with prettier
  • define an alias for the ./src/*
  • basic configuration for Chakra UI

The README file includes a description of good practices collected during the implementation of the ACRE project. This PR is in the draft and open to all suggestions to standardize our repositories. If you have any suggestions share them.

Copy link
Copy Markdown

@kpyszkowski kpyszkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left some comments:

Comment thread tsconfig.json
"baseUrl": "."
"baseUrl": ".",
"paths": {
"#/*": ["./src/*"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is alias prefixed with #? Any reason behind it? I've never seen such approach, the general widely used convention is to use @.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find @ confusing for internal aliases, as it is used as a prefix in external NPM package scopes.
# is a prefix that Node mentions in their documentation of subpath imports.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would capture each bit of reasoning in the commit messages, so that we can cross-reference. Usually I would say README, but since this is a template repo that'll be a potentially-strange starting point for a dApp's README.

Comment thread README.md Outdated

### pnpm

This project uses [pnpm](https://pnpm.io/) as a package manager. In many of our repositories we use a monorepo approach. The optimal solution for this approach is to use `pnpm`. It is faster than `npm` and consumes less disk storage than `yarn`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason to migrate from pnpm? It's stable, actively developed, much faster and pretty much innovate and efficient when it comes to memory management - node_modules has symbolic links to shared source of packages, local versioning of packages per case.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is because I hadn't committed to pnpm yet.

Consider this my commitment to pnpm, we can remove the commit that makes this switch and force-push it away.

Although y'all, if you haven't tried bun, that's some fast dependency resolution 😁 (We will not be switching to bun. Yet.)

Comment thread .prettierrc.js
@@ -0,0 +1,3 @@
module.exports = {
...require("@thesis-co/prettier-config"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see the only rule set in this config is semi with false value.
This rule is already defined by the default Prettier's config so it seems to be redundant. Unless it's planned to extend Thesis config with some additional rules?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our config precedes the default swap for prettier (which happened in this latest major version bump).

Let's light our custom prettier config on fire 💪

Comment thread src/DAppProviders.tsx
import theme from "./theme"
import DApp from "./DApp"

function DAppProviders() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered including such utility? The approach you propose has few drawbacks:

  • it will get difficult to read and maintain as applications scales:
<ProviderOne>
  <ProviderTwo>
    <ProviderThree>
      <ProviderFour>
        {...} 
      </ProviderFour>
    </ProviderThree>
  </ProviderTwo>
</ProviderOne>
  • in some cases the order of provider matters so some components might not have access to the data provided by specific provider

This simple utility resolves both problems. It can be written by hand - just a simple wrapper around reduce array method or installed as a package - there are a bunch of them available.

Comment thread README.md
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can include pre-commit hooks - describe how to install them in the readme and add a basic set of hooks

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

Successfully merging this pull request may close these issues.

5 participants