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

Update base template with latest hooks changes & disable gh-actions, precommit hook #2

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Jun 18, 2023

Description :

  1. Updated to generate deploy from inside we recently fixed it because ppl were having issue on Windows ( #362 on se-2 ) in the commit 859e75f
     
  2. Updated all the hooks to their latest version and also updated contracts.ts since we recently handled a beginner-friendly case on ( #372 in SE-2 ) in commit 96282fc
     
  3. disabled precommit and gh-actions in 6c9947e & 6c9947e
     

Regarding turning down some typescript / eslint error checks :

EslintConfig :

So basically we mainly rules coming from typecipt-eslint and eslint-plugin-next.

The default rules set up @typescript-eslint/recommended are checkout -> default @typescript-eslint/recommended config for their details checkout -> typescript-eslint rules detail.

I went through each of default config but I think we have already overridden the necessary defaults to make it beginner friendly the only update I did here was making no-unused-vars : warn instead of error

Regarding eslint-plugin-next checkout out defaults rules sets here -> default eslint-plugin-next rules
I think the defaults setup by plugin are sensible and might dont cause any harm to beginners. The no-img-element also gives just a warning which I think is fine.

TsConfig :

We have strict enabled in typescript and the default sets by strict are :

Please feel free to suggest enabling/disable any default both for eslint and tsconfig 🙌


ToDo's :

cc @carletex (Sorry for some reason I wasn't able to add reviewers 😅)

Note: Actual commits start from 859e75f (after the merge)...I think it's due to rebased and merged the previous PR

@technophile-04
Copy link
Collaborator Author

technophile-04 commented Jun 19, 2023

So after playing with different options I have dropped the idea of disabling noImplicitAny keeping tsConfig as it is.

TLDR;
If we want to disable noImplicitAny we have too disable strictNullChecks because if their values are different then the experience of beginner is ruined

So if we are disabling strictNullChecks and then only its worth disabling noImplicitAny, should we disbale strictNullChecks (I am kind of confused 😅 )?

checkout -> microsoft/TypeScript#21015 (comment)


To try yourself :

Just add in tsConfig under compilerOptions:

noImplicitAny : false, 

And then run yarn next:check-types -> this generates some types error which we don't get at SE-2 main branch. And this types error might frustrate begginer

if both noImplicitAny & strictNullChecks are set to false then we won't get any errors.

So should we disable strictNullChecks?


Also I think the PR is almost ready, @carletex could you try going through it once? Sorry for lots of changes it mainly due to 4839c8b (adding blockExplorer) commit and 5beb01e (adding MetaHeader), please let me if I should revert them and handle in different PR 🙌 (they are mainly copy-pastes with very little minor tweaks)

@technophile-04 technophile-04 requested a review from carletex June 19, 2023 18:24
@carletex
Copy link
Member

This is great job @technophile-04 . Thank you!!!

Regarding the Types... I'd say, let's merge this as it is, branch it for challenge 0, build, and see how it feels

@carletex carletex merged commit bd19973 into scaffold-eth:base-challenge-template Jun 21, 2023
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.

2 participants