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

Support strict type checking #177

Merged
merged 6 commits into from
Feb 3, 2020
Merged

Support strict type checking #177

merged 6 commits into from
Feb 3, 2020

Conversation

bjoluc
Copy link
Contributor

@bjoluc bjoluc commented Jan 24, 2020

I transitioned the whole project to strict type checking including noImplicitAny, which made me aware of a problem: The WrappedApp's constructor didn't pass the ctx to initStore. In fact, ctx was declared optional for initStore, but required by the MakeStoreOptions definition. This is now fixed.

Other notable changes:

  • Update the demo app.tsx to drop the deprecated NextJSAppProps
  • Add missing @types/react-test-renderer dev dependency

@bjoluc
Copy link
Contributor Author

bjoluc commented Jan 24, 2020

Also, regarding the "nasty hackery" in the client-side tests: Jest supports a jsdom environment, which we could use to get a real window object without hacking. Only requirement: Server and client tests have to be in separate files, since the Jest environment can only be set on a per-file basis. What do you think?

@bjoluc
Copy link
Contributor Author

bjoluc commented Jan 24, 2020

Another unrelated (and rather unimportant) thing I noticed: ESLint seems to ignore the .prettierrc configuration, making it throw errors for missing trailing commas. However, the commas are removed on-purpose by the esbenp.prettier-vscode plugin according to .prettierrc. In the end, Husky does the job via eslint --fix and makes eslint happy again. @kirill-konshin Do you think this is worth a fix?

Copy link
Owner

@kirill-konshin kirill-konshin left a comment

Choose a reason for hiding this comment

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

Very good job! The only issue is the props part.

packages/demo/pages/_app.tsx Outdated Show resolved Hide resolved
@kirill-konshin
Copy link
Owner

Technically since the repo is managed by ESLint, not prettier, you should not run any pure prettier plugins. I'm pushing a fix to make eslint to use prettier config more explicitly. Looks like something changed over time.

@kirill-konshin
Copy link
Owner

Also, regarding the "nasty hackery" in the client-side tests: Jest supports a jsdom environment, which we could use to get a real window object without hacking. Only requirement: Server and client tests have to be in separate files, since the Jest environment can only be set on a per-file basis. What do you think?

No objections. I'm pushing this now too. You will have to rebase your fork due to conflict.

Copy link
Owner

@kirill-konshin kirill-konshin left a comment

Choose a reason for hiding this comment

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

Very good job, thank you. Please rebase your fork since there are changes in fix-ts-typings that you are re-implementing.

Also I think we need to update readme with TS-friendly examples. We can do it in separate PR though.

I will release a new major version once we're done here.

packages/demo/pages/_app.tsx Outdated Show resolved Hide resolved
packages/wrapper/src/index.tsx Show resolved Hide resolved
@bjoluc
Copy link
Contributor Author

bjoluc commented Jan 31, 2020

Ok, I will create a separate readme PR after this, also tackling #170. Thanks for your time!

... to ReduxWrapperAppProps. Also:
* Remove legacy `AppProps` and `OriginalAppProps` alias
* Base `ReduxWrapperAppProps` directly on Next.js `AppProps`
@kirill-konshin kirill-konshin merged commit 4ecf6f5 into kirill-konshin:fix-ts-typings Feb 3, 2020
@kirill-konshin
Copy link
Owner

Merged. Thank you for your contribution. I will polish things here and there and release a new version soon. And separate kudos for answering questions in issues.

@bjoluc
Copy link
Contributor Author

bjoluc commented Feb 4, 2020

You're welcome!

@bjoluc bjoluc mentioned this pull request Feb 4, 2020
kirill-konshin added a commit that referenced this pull request Feb 11, 2020
Fix #165 
* TypeScript NextPageContext augmentation
* Fixed prettier
* JSDom tests in separate file
* Support strict type checking (#177)
* use strict type checking
* fix redux-related type annotations in demo
* Use single jest config
* Thanks to @jest-environment doc annotations; fixes coverage reports
* add ReduxWrappedAppProps interface
* Rename ReduxWrappedAppProps to ReduxWrapperAppProps
* Remove legacy `AppProps` and `OriginalAppProps` alias
* Base `ReduxWrapperAppProps` directly on Next.js `AppProps`
* Remove deprecated Container from example _app

Co-authored-by: bjoluc <mail@bjoluc.de>
kirill-konshin pushed a commit that referenced this pull request Feb 11, 2020
* Add motivation section (#170)
* Add TypeScript examples (#176, #177)
* Make installation code blocks expandable
* Migrate connected Page components to functional components
* Group tips and examples in a Tips and Tricks section
* Some grammar & style improvements
* Remove outdated typings reference in Resources section
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

2 participants