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

chore: wallet setup #25

Merged
merged 18 commits into from
Aug 1, 2023
Merged

chore: wallet setup #25

merged 18 commits into from
Aug 1, 2023

Conversation

dzbo
Copy link
Collaborator

@dzbo dzbo commented Jul 28, 2023

Ticket ID

DEV-6010

Description

Prepare repo and all deps/configs for new wallet.

@Hugoo
Copy link
Contributor

Hugoo commented Aug 1, 2023

don't you want to push this to the default branch instead (develop)?

@dzbo
Copy link
Collaborator Author

dzbo commented Aug 1, 2023

don't you want to push this to the default branch instead (develop)?

Yeah, I will push this to develop since now old wallet is in legacy branch.

@dzbo dzbo changed the base branch from up-wallet-beta-1.0 to develop August 1, 2023 09:19
@Hugoo
Copy link
Contributor

Hugoo commented Aug 1, 2023

Hmmm. do we need this?

image

/.output
/public/assets/fonts
/functions
/translations/
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be missing a .yarn here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot include .yarn here. Look at the universalprofile-extension repo and copy the yarn thing there or look at the docs. Without .yarn/plugins and .yarn/releases it will revert to yarn classic. The yarn 3 docs also have this documented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess here it's fine, we don't want to lint .yarn folder :D

Copy link
Contributor

@Hugoo Hugoo left a comment

Choose a reason for hiding this comment

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

nice, but i tihnk the .yarn folder is not needed

nuxt.config.ts Outdated
Comment on lines 21 to 23
plausible: {
domain: 'migrate.lukso.network',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not relevant anymore. We can remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah right domain is wrong, we not planning to use plausible for wallet?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can, ok lets add it!

"lint:fmt": "prettier --check --log-level=warn .",
"lint:fmt:fix": "prettier --write .",
"lint:types": "tsc --noEmit --project tsconfig.json",
"prepare": "husky install",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should install Husky.

Happy to discuss this but I think husky adds constraints and anyway, if the CI is correctly set up, we won;t push bad code to the repo.

Having husky adds constraints imo (commits are long to run because the stuff has to run, we can't push WIP commit or quickly change branches with "failing" changes...)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok np, I'm happy to remove it. Was re-using setup from migration repo

Copy link
Contributor

Choose a reason for hiding this comment

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

ok super, lets remove it then :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seem like it needs to stay, it's a part of the lint-staged installation and also because of what @richtera mentioned:

With yarn >3 you need to do the yarn husky install manually. Yarn no longer executes postinstall code in dependencies due to security.

Copy link
Contributor

Choose a reason for hiding this comment

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

lint-staged just speeds up dev by not having to wait a few minutes for results in gitops, but other than that they make things more involved when committing.

Comment on lines +73 to +77
"lint-staged": {
"*.{ts,js,vue}": [
"prettier --list-different",
"eslint"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of this lint staged thing... But up to you.

I rly think a good CI and branch protection is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

But lint staged is important otherwise you cannot partially commit things

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allow to catch issues earlier then on CI, I guess you can still run it from script but this is nice small automation that helps you not forget. If you are not liking this I will remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is really up to you, if you think it adds more value than "blocking time" then lets keep it.

@dzbo dzbo merged commit 4a9d353 into develop Aug 1, 2023
1 check passed
@dzbo dzbo deleted the chore/setup-DEV-6010 branch August 1, 2023 12:36
@richtera
Copy link
Contributor

richtera commented Aug 1, 2023

nice, but i tihnk the .yarn folder is not needed

You do need the .yarn folder. You can just exclude .yarn/cache if you don't want pnp.

@dzbo
Copy link
Collaborator Author

dzbo commented Aug 1, 2023

nice, but i tihnk the .yarn folder is not needed

You do need the .yarn folder. You can just exclude .yarn/cache if you don't want pnp.

Yes, .yarn folder stays, I've will remove it's cache tho, will be in next PR #27

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

Successfully merging this pull request may close these issues.

3 participants