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

feat: sample lib & app #6

Merged
merged 10 commits into from
Sep 11, 2024
Merged

feat: sample lib & app #6

merged 10 commits into from
Sep 11, 2024

Conversation

0xnigir1
Copy link
Collaborator

@0xnigir1 0xnigir1 commented Sep 10, 2024

Description

  • dummy example lib and app
  • lib contains a Provider that uses Viem to query account balances
  • app uses the library package for demo purposes
  • Vitest setup for testing
  • Clear README

Checklist before requesting a review

  • I have conducted a self-review of my code.
  • I have conducted a QA.
  • If it is a core feature, I have included comprehensive tests.


## Setup

1. Change package name to follow yours project one in [`package.json`](./package.json)

Choose a reason for hiding this comment

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

I didn't get "to follow yours project one," maybe you meant "Change package name to your own"

@@ -1,5 +1,5 @@
{
"extends": "./tsconfig.base.json",
"include": ["**/*", ".*.js"],
"exclude": ["node_modules", "dist", ".eslintrc.js"]

Choose a reason for hiding this comment

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

we could probably keep this in since eslint isn't relevant to the typescript compiler but either way it's ok

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since it's not that a big deal, i'll leave it as it is 👍

0xkenj1
0xkenj1 previously approved these changes Sep 10, 2024
Copy link

@0xkenj1 0xkenj1 left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines +4 to +9
"lint": {
"dependsOn": ["build"]
},
"lint:fix": {
"dependsOn": ["build"]
},
Copy link

Choose a reason for hiding this comment

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

Out of curiosity... Shouldn't this be the other way around? Build the "linted" code instead of linting the built code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure tbh, the CI issue not happening on local was because eslint errored on a type that couldn't resolve because the type was picked from another package of the monorepo, of course when already built, everything worked fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're suggesting that maybe the issue is not 'building -> lint' but rather some configuration on eslint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

the issue is this, if i run lint before building source code this error happens which makes sense to me

Copy link

Choose a reason for hiding this comment

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

Ooooh that makes sense. I was thinking task dependencies inside a non-monorepo.

What do you think about the following flow for each package:

  1. Build the "current" package dependencies
  2. Lint the "current" package
  3. Build the "current" package

Something like this:

Suggested change
"lint": {
"dependsOn": ["build"]
},
"lint:fix": {
"dependsOn": ["build"]
},
"lint": {
"dependsOn": ["^build"]
},
"lint:fix": {
"dependsOn": ["^build"]
},
"build": {
"dependsOn": ["^build", "lint"]
}

Copy link

Choose a reason for hiding this comment

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

So after reading the docs, the ^... dependsOn values also runs the "current" package task, which defeats the purpose of the general idea as we'd need to run a task only on the dependencies. Given that the current config works, I think that we can merge it and later come back to this if someone gets to know a way to do it.

@0xkenj1
Copy link

0xkenj1 commented Sep 11, 2024

Lets close this one as it is. On the following PR we will add:

  • Typechecks on the lint workflow

@0xnigir1 0xnigir1 merged commit 6f72565 into dev Sep 11, 2024
6 checks passed
@0xnigir1 0xnigir1 deleted the feat/sample-lib branch September 11, 2024 14:46
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.

4 participants