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/init nuxt app #68

Closed
wants to merge 14 commits into from
Closed

Chore/init nuxt app #68

wants to merge 14 commits into from

Conversation

dekanbro
Copy link

No description provided.

@dekanbro dekanbro requested a review from proofoftom June 30, 2020 15:01
@xuhcc
Copy link
Contributor

xuhcc commented Jul 2, 2020

@dekanbro Can you explain the purpose of this pull request? Since I'm also working on the frontend stuff, we have to coordinate in some way.

I don't really like the idea of rewriting everything from scratch. I also think that the existing tooling is better than the one you are proposing here (in particular, typescript would better suit our project, because MACI modules are written in typescript).

@proofoftom
Copy link
Member

proofoftom commented Jul 3, 2020

@xuhcc sorry for the lack of transparency as the decision happened in a different channel I think, am happy to invite you to the Raid Guild Discord channel as well. We've discussed and intend to bring over all of your work. The Nuxt decision was mine as I prefer its structure, magic, and support by the Vue community. I'm happy to implement typescript if you're familiar, I don't believe the two setups to be mutually exclusive, the reasoning for not including typescript was that neither @dekanbro or myself are familiar with it and it presented more of a hurdle than feature.

@xuhcc
Copy link
Contributor

xuhcc commented Jul 3, 2020

@proofoftom

We've discussed and intend to bring over all of your work.

Could you or @dekanbro rebase this branch onto xuhcc:show-recipients (#61)?

The Nuxt decision was mine as I prefer its structure, magic, and support by the Vue community.

OK, I'm not very familiar with Vue ecosystem so let's use Nuxt if you think it is better.

I'm happy to implement typescript if you're familiar, I don't believe the two setups to be mutually exclusive, the reasoning for not including typescript was that neither @dekanbro or myself are familiar with it and it presented more of a hurdle than feature.

Yeah, we already use it in the contracts app and in the MACI repo, and I think that type checks would be useful for frontend development as well. I also think that we should use the same tooling for backend and frontend to make collaboration easier, so I suggest replacing web3 with ethers if possible.

am happy to invite you to the Raid Guild Discord channel as well

I'd like to keep the discussions in our channels (although maybe I'll join Raid Guild someday 😉).

@xuhcc
Copy link
Contributor

xuhcc commented Jul 3, 2020

@dekanbro It seems that you did a merge instead of rebase, and for some reason GitHub now shows the diff incorrectly and it's hard to review. Please do a rebase:

git checkout chore/init-nuxt-app
git rebase master

In the end you should get clean linear history; then you can just force-push it. I just did it myself here: https://github.com/clrfund/monorepo/commits/nuxt

@dekanbro
Copy link
Author

dekanbro commented Jul 3, 2020

hmm i did do that. any other ideas?

@xuhcc
Copy link
Contributor

xuhcc commented Jul 3, 2020

Maybe we're using different versions of git (I'm on 2.27.0).
I think we can leave it as is and probably squash commits later when PR gets accepted. The issue with GitHub showing wrong commits is well-known, here's the solution that could work: https://stackoverflow.com/a/46782679/1868395

@proofoftom proofoftom changed the base branch from master to feature/appolo July 3, 2020 20:45
@proofoftom proofoftom changed the base branch from feature/appolo to master July 3, 2020 20:45
@xuhcc
Copy link
Contributor

xuhcc commented Jul 3, 2020

On the subject of PR:

  1. I still think that typescript is a must-have. I recommend switching to it right now, because if we don't do that, more work may be required as the codebase grows. I'm familiar with typescript and can help resolve any issues if they will arise (or I can just install it myself).
  2. I recommend replacing web3 with ethers because we already use it in contracts and as far as I know ethers is a good modern library and ethereum community is slowly migrating to it.
  3. Why did you remove the yarn.lock file? Currently we have single lockfile for both workspaces, and it seems to work fine. I suggest either restoring it in the root dir or creating a separate yarn.lock in the contracts workspace.
  4. Let's move vue-app/.editorconfig to the project's root.
  5. When I'm trying to run yarn install, I get this error error nanoid@3.1.10: The engine "node" is incompatible with this module. Expected version "^10 || ^12 || >=13.7". Got "11.15.0". We need to use node v11 because MACI modules require it.

(that's a preliminary review, I didn't yet actually run the code because of 5.)

@proofoftom
Copy link
Member

@xuhcc I followed your suggestion but somehow totally botched this, but it looks like you figured it out in https://github.com/clrfund/monorepo/commits/nuxt, why don't we just merge that?

@xuhcc
Copy link
Contributor

xuhcc commented Jul 3, 2020

@proofoftom no worries, I fixed it :)
You'll need to delete your local branch now and pull it from the repo.

@xuhcc
Copy link
Contributor

xuhcc commented Jul 4, 2020

3: yarn keeps creating the root yarn.lock when I run yarn install. It seems to be designed to work with single lockfile so we need to restore it.
5: nanoid "^3.1.10" requirement comes from the package called @nuxt/telemetry. We can probably use https://www.npmjs.com/package/nuxt-optout to replace @nuxt/telemetry.

@proofoftom
Copy link
Member

3: yarn keeps creating the root yarn.lock when I run yarn install. It seems to be designed to work with single lockfile so we need to restore it.
5: nanoid "^3.1.10" requirement comes from the package called @nuxt/telemetry. We can probably use https://www.npmjs.com/package/nuxt-optout to replace @nuxt/telemetry.

Yup, these both sound like good solutions to me 👍

@xuhcc
Copy link
Contributor

xuhcc commented Jul 9, 2020

@dekanbro Unfortunately nanoid dependency is still causing issues on Node v11. After some experimentation I found the solution, here is the pull request based on your branch: #73.

Fix nanoid dependency issue on Node v11
@proofoftom proofoftom closed this Jul 15, 2020
@xuhcc xuhcc mentioned this pull request Jul 31, 2020
@daodesigner daodesigner deleted the chore/init-nuxt-app branch December 16, 2021 21:54
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.

None yet

4 participants