Add support for Typescript to carbon/react #12222

mbarrer · 2022-10-04T14:16:11Z

Closes #11587
Closes #12654
Closes #12655
Closes #12656
Closes #12657

This PR adds support for compiling typescript files in the carbon/react package allowing us to begin converting components to typescript. As a proof of concept, Checkbox.js and all its imports have now been converted to typescript

Changelog

New

@babel/preset-typescript added to rollup and storybook builds to transpile ts
- Generates .js files to the output directory
@typescript-eslint/eslint-plugin and @typescript-eslint/parser added for linting
@rollup/plugin-typescript being used to perform typechecking on ts files
- This plugin will only output .d.ts files to the output directory, it does not handle generating .js (using emitDeclarationOnly: true compiler option)
Added TS extensions and @babel/preset-typescript to Jest config
Added types/common.d.ts with a newer typing for PolymorphicProps (components that use 'as' props to control the root node. additional explaination)
added types/globals.d.ts as a place to declare workspace globals

Changed

@types/react and @types/prop-types have been moved to resolutions and pinned to prevent pulling in types for react 18
Text.js, Checkbox.js and all their related components have been converted to TS

Testing / Reviewing

These changes can be tested and verified by running a fresh yarn, yarn build and yarn storybook. Verify that the behavior in both the Checkbox and Text stories remain unchanged

netlify · 2022-10-04T14:21:13Z

✅ Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`1b090eb`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-components-react/deploys/636040b71afc4b0009f68980
😎 Deploy Preview	https://deploy-preview-12222--carbon-components-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify · 2022-10-04T14:22:43Z

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	`1b090eb`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/636040b7682f360008d0a040
😎 Deploy Preview	https://deploy-preview-12222--carbon-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

tay1orjones

Thanks for this! Here's a few baseline thoughts from my first pass through.

package.json

tay1orjones · 2022-10-05T15:11:44Z

packages/react/.storybook/main.js

@@ -92,6 +92,9 @@ module.exports = {
  },
  framework: '@storybook/react',
  stories,
+  typescript: {
+    reactDocgen: 'react-docgen', // Favor docgen from prop-types instead of TS interfaces


I was going to ask about this. I agree and think we should favor the docgen from prop-types for now. Maybe once things are fully converted we could evaluate using the TS interfaces instead.

tay1orjones · 2022-10-05T18:16:13Z

packages/react/src/components/Checkbox/Checkbox.tsx

+  /**
+   * Specify a title for the <label> node for the Checkbox
+   */
+  title: PropTypes.string,


This one doesn't have a type supplied in the interface above. Is that intentional?

Correct this is intentional. The reason is because since we are doing the spread of ...others on the input element I have decided to have the interface extend all the native props of an HTMLInputElement using React.InputHTMLAttributes<HTMLInputElement>. We omit some native props that we are overriding/providing stricter types for

tay1orjones · 2022-10-05T20:09:59Z

packages/react/src/components/Text/TextDirectionContext.ts

+export interface TextDirectionContextData {
+  direction?: TextDir;
+  getTextDirection?: MutableRefObject<GetTextDirection | undefined>;
+}
+
+export const TextDirectionContext = createContext<TextDirectionContextData>({});
+export const useTextDirectionContext = () =>
+  useContext<TextDirectionContextData>(TextDirectionContext);


IIUC the change here from the original file is largely because we need to have typings for everything that is placed within the context. Is that correct? It's a big stylistic change that we'll need to make for any place we use context. Tabs in particular could be a complicated refactor. Without this change does compilation fail, or is it just a warning?

I'm not sure if initializing the context to an empty object instead of null has any side effects. I can't immediately think of any though.

Yes this is correct, the refactor here was so that we can have all the types for the context in one place. The addition of the useTextDirectionContext was to provide a convenience method for the useContext so we didnt have to manually pass the typing to context every time. The empty object instantiation here is from a pattern we use over in fusion-ui, since this context isnt really used too widely I was able to replace the null here with just an empty object. For other contexts that have wider usages, we can probably just incorporate null as a valid type for the contexts to prevent bigger refactors

If we want to go more conservatively here, we could "not" do types for the context and then just use any here (or something similar).

If the goal is to provide types externally for carbon/react, then this conversion of the context doesn't actually help that goal (I'm assuming this context isn't exposed externally).

I know it's tempting to type everything that's encountered along the way, but that's likely not in the best interest of the Carbon System Squad right now until the team gets more comfortable developing in typescript.

A type is required for the useContext otherwise when its used we get an object with an any type and have no idea what properties are valid for this context

I definitely get that, and in a totally typescript product I'd 100% agree this is the correct approach, but that's unfortunately not the state of carbon/react ...yet!

My argument is that the lack of typing on that context is no worse than what is currently present in the JS codebase. The existing code functions as-is without types. I definitely believe that converting all the things is a great eventual goal, but I don't believe that now is necessarily the right time for that.

In a general sense, what the System squad has the capacity to take on at this point maintenance and new-development-wise is the introduction of type interfaces at the externally-exposed component level, but not anything beyond that at this time. That's why I'm erring more on the side of caution with respect to the scope of things we convert to typescript out of the gate.

Im a little confused, are you suggesting that we not convert this file to TS at all or just remove the interface and change it to any? Im not sure how the addition of the interface is going to cause confusion in the JS code, I would actually think that it helps set the expectations on what we are able to access and set. Are we concerned that a developer might not necessarily know that they have to add a prop to the interface if we wanted to add something new to a context?

@jdharvey-ibm Are you saying you'd rather

export interface TextDirectionContextData { direction?: TextDir; getTextDirection?: MutableRefObject<GetTextDirection | undefined>; [key: string]: any; }

So they can pass in additional unknown arguments?

@mbarrer sorry I missed your comment otherwise I would have responded sooner!

I'd recommend we don't convert any files to TS unless they are an exported component and are part of the public interface of the carbon/react package. In this case, that would mean leaving the TextDirectionContext file as a JS file and asserting the type of that context in places where it is used as any or something else that's equally basic.

You're 100% correct that it's ambiguous what properties exist on the context and that it is (generally speaking) an improvement to the codebase to assign types to that context and other non-component files.

But with that said, the goal of this particular work item is to get component types to the community and to get them out there as quickly as possible. Thereby unblocking the transition to v11 and all its goodies. Making changes to any non-external-component file is useful, but it unfortunately doesn't expedite that goal.

Once we get the initial typings done, a great next step would be to start improving on these sorts of things to make the internals more bullet-proof.

erifsx

This looks great to me!

tay1orjones

Thanks for making those changes!

kodiakhq · 2022-10-28T19:50:04Z

This PR currently has a merge conflict. Please resolve this and then re-add the status: ready to merge 🎉 label.

gi · 2022-11-03T01:04:48Z

With this merged, will the team start or like help converting component files to Typescript?

tay1orjones · 2022-11-15T19:40:57Z

@gi sorry for the delayed response - yes! I've begun organizing an umbrella issue #12513 containing all the pieces needed for this so we can track overall status on component-by-component basis.

I'll be putting out a formal call for contributions soon. We're still ironing out some kinks, #12600

mbarrer added 2 commits October 4, 2022 09:03

refactor: add build support for TS

9ebbc29

refactor: convert Checkbox and Text to typescript

fcc9f98

mbarrer requested a review from a team as a code owner October 4, 2022 14:16

mbarrer requested review from tw15egan and jnm2377 October 4, 2022 14:16

mbarrer added 3 commits October 4, 2022 10:50

refactor: add ts extensions to jest

6b58f27

refactor: add ts extensions to jsTransform.js

4241aeb

refactor: add type exports to barrels

3bd84d9

tay1orjones requested review from erifsx, jharvey10, metonym and tay1orjones October 5, 2022 15:08

tay1orjones reviewed Oct 6, 2022

View reviewed changes

erifsx approved these changes Oct 7, 2022

View reviewed changes

mbarrer added 5 commits October 17, 2022 10:28

refactor: do not convert TextDirectionContext to TS

12b4437

chore: add comment

bbd77f8

refactor: update Text with context

219d356

refactor: use @type jsdoc assertion

29e095d

refactor: fix build and turn off explicit any rule

88daf70

tay1orjones approved these changes Oct 19, 2022

View reviewed changes

This was referenced Oct 20, 2022

Support TypeScript in our Storybook to set a baseline for types #11587

Closed

docs(semver): add statement of support for typescript #12357

Merged

Semigradsky mentioned this pull request Oct 24, 2022

Missing "Stack" component DefinitelyTyped/DefinitelyTyped#62842

Closed

jharvey10 approved these changes Oct 25, 2022

View reviewed changes

tw15egan approved these changes Oct 27, 2022

View reviewed changes

metonym approved these changes Oct 27, 2022

View reviewed changes

tay1orjones added the status: ready to merge 🎉 label Oct 28, 2022

kodiakhq bot removed the status: ready to merge 🎉 label Oct 28, 2022

tay1orjones added the status: merge conflict 🚫 label Oct 28, 2022

mbarrer force-pushed the mbarrer branch from 1f405af to 88daf70 Compare October 31, 2022 21:28

mbarrer added 3 commits October 31, 2022 16:34

chore: merge

406d89e

chore: yarn dedup

00eed65

chore: run install withh yarn 3.2.4

1b090eb

tay1orjones added status: ready to merge 🎉 and removed status: merge conflict 🚫 labels Nov 1, 2022

kodiakhq bot merged commit fa262d5 into carbon-design-system:main Nov 1, 2022

tay1orjones mentioned this pull request Nov 1, 2022

Convert packages/react storybook to typescript. Convert checkbox story to tsx #12000

Closed

tay1orjones mentioned this pull request Nov 7, 2022

Add TypeScript types to @carbon/react components #12513

Closed

This was referenced Nov 15, 2022

Add TypeScript types to Text, TextDirection #12657

Closed

Add TypeScript types to Checkbox #12654

Closed

Add TypeScript types to CheckboxSkeleton #12655

Closed

Add TypeScript types to usePrefix #12656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Typescript to carbon/react #12222

Add support for Typescript to carbon/react #12222

mbarrer commented Oct 4, 2022 •

edited by tay1orjones

Loading

netlify bot commented Oct 4, 2022 •

edited

Loading

netlify bot commented Oct 4, 2022 •

edited

Loading

tay1orjones left a comment

tay1orjones Oct 5, 2022

tay1orjones Oct 5, 2022

mbarrer Oct 6, 2022

tay1orjones Oct 5, 2022

mbarrer Oct 6, 2022 •

edited

Loading

jharvey10 Oct 6, 2022

mbarrer Oct 6, 2022

jharvey10 Oct 6, 2022

mbarrer Oct 6, 2022

erifsx Oct 7, 2022 •

edited

Loading

jharvey10 Oct 11, 2022

erifsx left a comment

tay1orjones left a comment

kodiakhq bot commented Oct 28, 2022

gi commented Nov 3, 2022

tay1orjones commented Nov 15, 2022

Add support for Typescript to carbon/react #12222

Add support for Typescript to carbon/react #12222

Conversation

mbarrer commented Oct 4, 2022 • edited by tay1orjones Loading

Changelog

Testing / Reviewing

netlify bot commented Oct 4, 2022 • edited Loading

✅ Deploy Preview for carbon-components-react ready!

netlify bot commented Oct 4, 2022 • edited Loading

✅ Deploy Preview for carbon-elements ready!

tay1orjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbarrer Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erifsx Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erifsx left a comment

Choose a reason for hiding this comment

tay1orjones left a comment

Choose a reason for hiding this comment

kodiakhq bot commented Oct 28, 2022

gi commented Nov 3, 2022

tay1orjones commented Nov 15, 2022

mbarrer commented Oct 4, 2022 •

edited by tay1orjones

Loading

netlify bot commented Oct 4, 2022 •

edited

Loading

netlify bot commented Oct 4, 2022 •

edited

Loading

mbarrer Oct 6, 2022 •

edited

Loading

erifsx Oct 7, 2022 •

edited

Loading