feat: add typescript support #758

ghost · 2021-03-18T15:58:51Z

Resolves #709
Impact: major
Type: feature

Issue

DX Suffers without TypeScript

Solution

Add strict TypeScript support

Breaking changes

None, though I've changed the code style a little to use functional components, added Prettier to the mix and disabled some ESLint rules in TypeScript files. If we're going to discuss code style now would be a good time. Setting some established patterns now will help others who plan to perform a TypeScript conversion as they fork their Storefronts.

Configuration

Configure VSCode to use Prettier for TypeScript files by opening the Command Palette (Cmd+Shift+P), opening the Settings JSON file and adding:

"editor.formatOnSave": true,
"[typescript]": {
  "editor.defaultFormatter": "esbenp.prettier-vscode"
},
"[typescriptreact]": {
  "editor.defaultFormatter": "esbenp.prettier-vscode"
},

Then run ext install esbenp.prettier-vscode to install it.

Testing

Checkout this branch
Run yarn to install dependencies
Run the app
Confirm header looks the same as before
Run yarn type-check from the command line
Confirm there are exactly 0 type issues
Review Header.tsx
Confirm it a functional component written in type-script
Open the file in VSCode
Confirm no ESLint issues appear in the file
Observe the TODO item for Link component
Attempt to add a MUI component anywhere in the JSX before it's imported
Observe VSCode IntelliSense is working and the import statement is added automatically

/cc @janus-reith @nnnnat

janus-reith · 2021-03-23T18:09:00Z

types/reaction.d.ts

@@ -0,0 +1 @@
+declare module "@reactioncommerce/components/ShopLogo/v1";


Do we need this?

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).

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.

janus-reith · 2021-03-23T18:12:43Z

components/Header/Header.tsx

+  viewer: any;
+}
+
+const Header: FC<HeaderProps> = ({ classes, shop, uiStore }) => {


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, FC seems to add no value so we could use const 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.

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.

const Header = ({ classes: { toolbar, title, controls, appBar }, shop: { name = "cmVhY3Rpb24vc2hvcDo5TVNZOEZ4ZFI2dTJaOHVNdg==" }, uiStore, }: HeaderProps) => {

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:

const Header = ({ classes: { toolbar, title, controls, appBar }, shop: { name = "cmVhY3Rpb24vc2hvcDo5TVNZOEZ4ZFI2dTJaOHVNdg==" }, uiStore, }: { shop: { name: string; }; uiStore: { toggleMenuDrawerOpen: Function; }; viewer: any; }) => {

Then there's the moment where you want to return null inside an element—likely with a ternary expression—and are required to add children to the type definition and you'll still get an error until you choose the correct return type (which FC already does for us).

Type 'Element | null' is not assignable to type 'HeaderProps'. Type 'null' is not assignable to type 'HeaderProps'.ts(2322)

The biggest disadvantage I see of using FC has to do with potentially adding children where 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 FC to 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?

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

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.

janus-reith · 2021-03-23T18:25:35Z

Thanks for getting this started @balibebas!

ghost · 2021-03-31T08:34:45Z

@janus-reith Just saw your code review. I'll loop back to this soon.

ghost

@janus-reith all set. once you've considered my responses I'm comfortable moving forward with either grammar for the function signatures so we can get this code merged.

Akarshit · 2021-04-01T14:31:52Z

Let me know if this looks good, and I can get it merged.

ghost · 2021-04-01T15:46:19Z

package.json

+        "rules": {
+          "comma-dangle": "off",
+          "react/prop-types": "off",
+          "quote-props": "off"


Two more eslint rules worth disabling now as prettier takes care of it:

"react/jsx-max-props-per-line": "off", "max-len": "off"

I'll plan to add these as I incorporate review feedback. 🙏🏼

janus-reith · 2021-04-05T09:53:05Z

tsconfig.json

@@ -0,0 +1,21 @@
+{
+  "compilerOptions": {
+    "target": "es5",


Should we use "es5" here?
The next's typescript example uses target es next: https://github.com/vercel/next.js/blob/canary/examples/with-typescript/tsconfig.json
and next's itself uses ES2017: https://github.com/vercel/next.js/blob/canary/packages/next/tsconfig.json

TBH I'm unsure what the actual effect of different values here is, since by my understanding, nextjs would not really compile typescript, but typecheck on build and then leverage babel and @babel/preset-typescript to just strip out type definitions.

Seems reasonable to shift forward. I suspect it'll reduce bundle sizes by removing boilerplate for things like generator functions where async could be used.

janus-reith

Looks good to me, thanks @balibebas!

I'd like to reconsider what target we set in tsconfig, but approving for now since my by understanding, impact of that property is rather low for us anyways.

Next step might be introducing https://github.com/dotansimha/graphql-code-generator to derive most types from our api schema. By leveraging the typescript-react-apollo plugin and defining some operations (out of the gql files we have already) we can even auto-generate typed apollo hooks for Querys and Mutations and save us a lot of work.

Signed-off-by: Josh Habdas <jhabdas@protonmail.com>

ghost · 2021-04-05T12:16:13Z

@Akarshit we're all set once ci checks have passed. thanks @janus-reith for your feedback and review

nnnnat · 2021-04-06T13:58:21Z

LGTM! If @janus-reith and @Akarshit are good with this let merge

Akarshit · 2021-04-07T06:49:22Z

I will give this one a last look tomorrow and then merge it.

Akarshit · 2021-04-08T08:10:24Z

I didn't get much time these days, but I will definitely get this done tomorrow.

ghost · 2021-04-09T06:40:48Z

Backing down a change made during review to prevent non-blocking test warning given the TypeScript target esnext isn't fully supported by Node 12, the current engine running in the docker container. es2019 works fine for Node 12.

Prevents warning on Jest test runs:

ts-jest[config] (WARN) There is a mismatch between your NodeJs version v12.14.1 and your TypeScript target ESNext. This might lead to some unexpected errors when running tests with `ts-jest`. To fix this, you can check https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping

See Node target mappings here: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping

Akarshit · 2021-04-09T07:17:00Z

@balibebas Hey! looks like to forgot to do the sign-off for the last commit...

Signed-off-by: Josh Habdas <jhabdas@protonmail.com>

ghost · 2021-04-09T07:48:02Z

@balibebas Hey! looks like to forgot to do the sign-off for the last commit...

Nice catch. Done

Akarshit · 2021-04-12T18:35:49Z

@balibebas Hey! Sorry it was the weekend so I couldn't get to this one. Looks really good to me. Thanks a lot 🎉

rc-publisher · 2021-06-23T00:57:35Z

🎉 This PR is included in version 4.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

janus-reith reviewed Mar 23, 2021

View reviewed changes

ghost commented Apr 1, 2021

View reviewed changes

Akarshit self-requested a review April 1, 2021 14:31

ghost commented Apr 1, 2021

View reviewed changes

ghost requested a review from janus-reith April 5, 2021 07:03

janus-reith reviewed Apr 5, 2021

View reviewed changes

janus-reith approved these changes Apr 5, 2021

View reviewed changes

feat: add typescript support

bcf17cd

Signed-off-by: Josh Habdas <jhabdas@protonmail.com>

refactor: 💡 prevent jest warn due to typescript target

38e5ee7

Signed-off-by: Josh Habdas <jhabdas@protonmail.com>

Akarshit approved these changes Apr 12, 2021

View reviewed changes

Akarshit merged commit 78681a4 into reactioncommerce:trunk Apr 12, 2021

ghost deleted the tscftw branch April 13, 2021 10:33

rc-publisher added the released Applied automatically by semantic-release label Jun 23, 2021

		@@ -0,0 +1 @@
		declare module "@reactioncommerce/components/ShopLogo/v1";

feat: add typescript support #758

feat: add typescript support #758

Uh oh!

Conversation

ghost commented Mar 18, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Breaking changes

Configuration

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janus-reith Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janus-reith commented Mar 23, 2021

Uh oh!

ghost commented Mar 31, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Akarshit commented Apr 1, 2021

Uh oh!

ghost Apr 1, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janus-reith left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 5, 2021

Uh oh!

nnnnat commented Apr 6, 2021

Uh oh!

Akarshit commented Apr 7, 2021

Uh oh!

Akarshit commented Apr 8, 2021

Uh oh!

ghost commented Apr 9, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akarshit commented Apr 9, 2021

Uh oh!

ghost commented Apr 9, 2021

Uh oh!

Akarshit commented Apr 12, 2021

Uh oh!

rc-publisher commented Jun 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Mar 18, 2021 •

edited by ghost

Loading

janus-reith Mar 23, 2021 •

edited

Loading

ghost Apr 1, 2021 •

edited by ghost

Loading

janus-reith left a comment •

edited

Loading

ghost commented Apr 9, 2021 •

edited by ghost

Loading