feat: add typescript support #758
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| build/* | ||
| node_modules/* | ||
| reports/* | ||
| /**/*.d.ts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "trailingComma": "all", | ||
| "tabWidth": 2, | ||
| "semi": true, | ||
| "singleQuote": false, | ||
| "bracketSpacing": true, | ||
| "arrowParens": "always" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import React from "react"; | ||
| import inject from "hocs/inject"; | ||
| import AppBar from "@material-ui/core/AppBar"; | ||
| import Hidden from "@material-ui/core/Hidden"; | ||
| import Toolbar from "@material-ui/core/Toolbar"; | ||
| import Typography from "@material-ui/core/Typography"; | ||
| import { createStyles, withStyles } from "@material-ui/core/styles"; | ||
| import { NavigationDesktop } from "components/NavigationDesktop"; | ||
| import { | ||
| NavigationMobile, | ||
| NavigationToggleMobile, | ||
| } from "components/NavigationMobile"; | ||
| import LocaleDropdown from "components/LocaleDropdown"; | ||
| import AccountDropdown from "components/AccountDropdown"; | ||
| import ShopLogo from "@reactioncommerce/components/ShopLogo/v1"; | ||
| import Link from "components/Link"; | ||
| import MiniCart from "components/MiniCart"; | ||
|
|
||
| import type { FC } from "react"; | ||
| import type { WithStyles, Theme } from "@material-ui/core"; | ||
|
|
||
| const styles = (theme: Theme) => | ||
| createStyles({ | ||
| appBar: { | ||
| backgroundColor: theme.palette.reaction.white, | ||
| borderBottom: `solid 1px ${theme.palette.reaction.black05}`, | ||
| color: theme.palette.reaction.coolGrey500, | ||
| }, | ||
| controls: { | ||
| alignItems: "inherit", | ||
| display: "inherit", | ||
| flex: 1, | ||
| }, | ||
| title: { | ||
| color: theme.palette.reaction.reactionBlue, | ||
| marginRight: theme.spacing(), | ||
| borderBottom: `solid 5px ${theme.palette.reaction.reactionBlue200}`, | ||
| }, | ||
| toolbar: { | ||
| alignItems: "center", | ||
| display: "flex", | ||
| justifyContent: "space-between", | ||
| }, | ||
| }); | ||
|
|
||
| interface HeaderProps extends WithStyles<typeof styles> { | ||
| shop: { | ||
| name: string; | ||
| }; | ||
| uiStore: { | ||
| toggleMenuDrawerOpen: Function; | ||
| }; | ||
| viewer: any; | ||
| } | ||
|
|
||
| const Header: FC<HeaderProps> = ({ classes, shop, uiStore }) => { | ||
| const handleNavigationToggleClick = () => { | ||
| uiStore.toggleMenuDrawerOpen(); | ||
| }; | ||
| return ( | ||
| <AppBar position="static" elevation={0} className={classes.appBar}> | ||
| <Toolbar className={classes.toolbar}> | ||
| <Hidden mdUp> | ||
| <NavigationToggleMobile onClick={handleNavigationToggleClick} /> | ||
| </Hidden> | ||
|
|
||
| <div className={classes.controls}> | ||
| <Typography className={classes.title} color="inherit" variant="h6"> | ||
| {/* @ts-ignore TODO: Refactor link to address type error */} | ||
| <Link route="/"> | ||
| {shop ? <ShopLogo shopName={shop.name} /> : "Example Storefront"} | ||
| </Link> | ||
| </Typography> | ||
|
|
||
| <Hidden smDown initialWidth={"md"}> | ||
| <NavigationDesktop /> | ||
| </Hidden> | ||
| </div> | ||
|
|
||
| <LocaleDropdown /> | ||
|
|
||
| <AccountDropdown /> | ||
| <MiniCart /> | ||
| </Toolbar> | ||
| <NavigationMobile shop={shop} /> | ||
| </AppBar> | ||
| ); | ||
| }; | ||
|
|
||
| export default withStyles(styles)(inject("uiStore")(Header)); | ||
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| { | ||
| "compilerOptions": { | ||
| "target": "es2019", | ||
| "lib": ["dom", "dom.iterable", "esnext"], | ||
| "allowJs": true, | ||
| "skipLibCheck": true, | ||
| "strict": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "importsNotUsedAsValues": "error", | ||
| "noEmit": true, | ||
| "esModuleInterop": true, | ||
| "module": "esnext", | ||
| "moduleResolution": "node", | ||
| "resolveJsonModule": true, | ||
| "isolatedModules": true, | ||
| "jsx": "preserve", | ||
| "baseUrl": "." | ||
| }, | ||
| "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", "./types/**/*"], | ||
| "exclude": ["node_modules"] | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import "@material-ui/core/styles/createPalette"; | ||
|
|
||
| declare module "@material-ui/core/styles/createPalette" { | ||
| interface Palette { | ||
| reaction: any; | ||
| } | ||
| interface PaletteOptions { | ||
| reaction?: any; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| declare module "@reactioncommerce/components/ShopLogo/v1"; | ||
|
Author
There was a problem hiding this comment. This prevents a type error using modules from the component library. This will eventually become a list of items as they are used from the lib, each added when it becomes used. Not the most straight-forward thing but to me sane until there's a better approach (I haven't found one yet).
Collaborator
There was a problem hiding this comment. Ok yes I was also expecting this to become a larger list at some point and therefore wondered if we couldn't avoid that somehow. But IMHO not a blocker, something that could easily be refactored later on. |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to discuss the general style we adopt here:
Typescript could infer these, should we utilize that and reduce boilerplate, or do we want to be explicit all the time?
Also,
FCseems to add no value so we could useconst Header = ({ classes, shop, uiStore }: HeaderProps)which I would prefer personally.I don't want to pick on this specific one as it's just an implementation example, but it's style will potentially be adopted in other places aswell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking behind use of Type Parameters is to identify type immediately when looking at a function without having parse my eye parse over an argument and possible default params before getting to it, e.g.
Add an explicit return type, assign default args or define the types inline and it personally starts to give me the bug eyes even after Prettier has gone to work on it, and still suffers from the eye scanning problem I mentioned:
Then there's the moment where you want to return
nullinside an element—likely with a ternary expression—and are required to addchildrento the type definition and you'll still get an error until you choose the correct return type (whichFCalready does for us).The biggest disadvantage I see of using
FChas to do with potentially addingchildrenwhere we're not dealing with a react component, in which case I prefer the grammar you supplied.That said it seems as though others prefer omitting
FCto save a little boilerplate and I'm cool with that so long as we can agree on a way forward to get the migration started and let the style grow out of the refactored code before adding any opinion.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, here's the number of times we're returning null in component definitions which can be used as a rough proxy for how often the above error will occur as people are coding: https://github.com/reactioncommerce/example-storefront/search?q=%22return+null%22&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to explain your thoughts on this!
I'm happy to keep it this way.
Longer-term if we deal with passing more data types from the api (which hopefully we won't all type by hand within components anyway and just use codegen) we can still the what patterns fits best, so I'm negating my original statement, not a blocker and nothing that needs to be set in stone right now.