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

Turn on noImplicitAny and strictPropertyInitialization for fast-router #5341

Open
chrisdholt opened this issue Oct 28, 2021 · 15 comments
Open
Assignees
Labels
area:fast-router Pertains to fast-router community:good-first-issue Good issues for first time contributors improvement A non-feature-adding improvement
Milestone

Comments

@chrisdholt
Copy link
Member

As part of #5303 we turned off @typescript-eslint/typedef globally as it was overwritten across multiple packages. As part of that, we turned on noImplicitAny and strictPropertyInitialization for all packages. Turning this on revealed several issues related to both tsconfig settings in fast-router and so it was turned off explicitly for that package. The errors should be fixed and addressed as part of this issue and the code should be tested to ensure that there are no breaks or side effects of the change. This issue exists to track turning that back on and resolving the errors.

@chrisdholt chrisdholt added improvement A non-feature-adding improvement area:fast-router Pertains to fast-router community:good-first-issue Good issues for first time contributors labels Oct 28, 2021
@ritikBhandari
Copy link

Hi @chrisdholt,
So as far as I can understand, while turning on noImplicitAny and strictPropertyInitialization, the issues related to both tsconfig settings in fast-router also need to be resolved. Right?

@chrisdholt
Copy link
Member Author

Hi @chrisdholt, So as far as I can understand, while turning on noImplicitAny and strictPropertyInitialization, the issues related to both tsconfig settings in fast-router also need to be resolved. Right?

Yes, the goal for this would be to turn those settings on and resolve the issues related to them for the fast-router package.

@ritikBhandari
Copy link

ritikBhandari commented Mar 12, 2022

Would u mind if I try to resolve this issue?

@ritikBhandari
Copy link

Turning this on revealed several issues related to both tsconfig settings in fast-router

@chrisdholt how should I reproduce these errors again? Did u find it in testing phase or any other case?

@chrisdholt
Copy link
Member Author

Turning this on revealed several issues related to both tsconfig settings in fast-router

@chrisdholt how should I reproduce these errors again? Did u find it in testing phase or any other case?

If I recall, the issue should repro by turning on the settings in tsconfig and running the build and tests steps.

@ritikBhandari
Copy link

ritikBhandari commented Mar 19, 2022

During testing, all test cases are running except for fast-router. Is that the issue? Because right now lerna run test is working fine except for that particular folder.

@chrisdholt
Copy link
Member Author

During testing, all test cases are running except for fast-router. Is that the issue?

No the issue is you would see errors thrown relating to instances of no-implicit-any and several cases where strictPropertyInitialization isn’t being done as expected. Not at my computer but I can try to give an example tomorrow (maybe this evening but will be afk for a bit).

@ritikBhandari
Copy link

Sure.
In the meanwhile i'll check why the tests are running with exception.

@ritikBhandari
Copy link

@chrisdholt can you attach any example of the errors as I still cannot find them after turning on those parameters.

@ritikBhandari
Copy link

ritikBhandari commented Mar 26, 2022

fasterror

@chrisdholt Are we noticing these type of errors?

@chrisdholt
Copy link
Member Author

fasterror

@chrisdholt Are we noticing these type of errors?

That looks like what I would reproduce - those appear to be noImplicitAny errors.

@ritikBhandari
Copy link

So now we're sure these are the errors to be fixed right?

@chrisdholt
Copy link
Member Author

So now we're sure these are the errors to be fixed right?

Yes, we want to fix the noImplicitAny typings, which is likely going to require adding the correct typings. That could open up potential type issues with the implementation where something may be inferred but could be undefined or may not overlap.

Probably best to get @EisenbergEffect to confirm, but I think a PR here for stricter typing would be an improvement, it'll likely just take some time to review depending on what all gets flagged as you move through fixing these.

@EisenbergEffect
Copy link
Contributor

The router is still in a bit of a volatile stage so I haven't added all the typings. It can be easier to evolve something at first without that, adding it later once the code has matured. That said, I'd definitely take a PR to improve this now.

@NiteshSethi09
Copy link
Contributor

Hey, is there anyone working on this issue? I'd like to give a try. @EisenbergEffect @chrisdholt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-router Pertains to fast-router community:good-first-issue Good issues for first time contributors improvement A non-feature-adding improvement
Projects
Status: Triage
Development

No branches or pull requests

4 participants