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: eslint + Typescript config #3

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

johnvente
Copy link
Contributor

Configuration for Eslint and Typescript

Eslint: It has some rules React app code

Two new scripts:

  • lint to check if there any error

     yarn lint
    
  • lint-fix to fix the errors found

     yarn lint
    

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

@mariajgrimaldi
Copy link
Contributor

We should have a GH action that runs yarn lint then, shouldn't we?

@johnvente
Copy link
Contributor Author

johnvente commented Sep 4, 2023

We should have a GH action that runs yarn lint then, shouldn't we?

Yep, we should have it, I'll add it
@mariajgrimaldi done!

@mariajgrimaldi
Copy link
Contributor

Also, why do we need typescript? Isn't that adding more complexity to the implementation?

@johnvente
Copy link
Contributor Author

Also, why do we need typescript? Isn't that adding more complexity to the implementation?

I will need to type props for the components, I can use prop-types lib but it will be better to use Typescript for maintaining this and for types in general. It wouldn't add more complexity

@johnvente johnvente force-pushed the jv/eslint-typescript-config branch 2 times, most recently from 3e9891d to 8743a32 Compare September 4, 2023 21:27
@mariajgrimaldi
Copy link
Contributor

If you don't mind me asking, why are prop types needed?

@johnvente
Copy link
Contributor Author

If you don't mind me asking, why are prop types needed?

When we upload a file, there will be a list of images each image will be a component (needs props), list of images will be another component (needs prop with the list of images). It's a good practice to type props and types that in this case I will use a context as well

@mariajgrimaldi
Copy link
Contributor

mariajgrimaldi commented Sep 4, 2023

But does the library need types? Is adding typescript making the library more or less maintainable? Can we elaborate on it?
cc @felipemontoya

@johnvente
Copy link
Contributor Author

maintainable

Typescript makes the React app more maintainable, there is not a problem adding it. Why is there problem with this?

@felipemontoya
Copy link
Member

Because we have so far had problems with every extra type of javascript language that we add to the stack. Adding coffescript was a nightmare when it promised to make everything easier. We are already suspicious of adding new variants of Javascript. It usually makes whatever we are doing harder to maintain in the long term.

@mariajgrimaldi
Copy link
Contributor

I'm gonna move this back to draft in case we need it in the future

@mariajgrimaldi mariajgrimaldi marked this pull request as draft September 7, 2023 13:26
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.

3 participants