refactor(icons-react): add TypeScript types to icons-react package #14714

lewandom · 2023-09-26T08:14:15Z

Closes #11317

Adds TypeScript types generation to the @carbon/icons-react package and to @carbon/react/icons submodule.

Changelog

New

TypeScript bundler in @carbon/cli
Dynamic generation of TypeScript type definitions for icons exported by @carbon/icons-react

Changed

Icon component migrated to TypeScript
react and reactNext builders in @carbon/icon-build-helpers switched to compiling TypeScript code
icon-helpers package converted to TypeScript

Testing / Reviewing

Icons rendering in Storybook (e.g. in Accordion or FileUploader components) should work without any visual and behavioral changes. In a TypeScript environment, you should be able to import and reference icons using @carbon/react/icons module, with props auto-completion working.

netlify · 2023-09-26T08:19:40Z

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`29c685c`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/65321d3358113e0008ff836f
😎 Deploy Preview	https://deploy-preview-14714--v11-carbon-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 configuration.

netlify · 2023-09-26T08:22:07Z

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	`29c685c`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/65321d33f92c8100080ce715
😎 Deploy Preview	https://deploy-preview-14714--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 configuration.

lewandom · 2023-09-26T08:35:59Z

@jdharvey-ibm @metonym @joshblack @tay1orjones you may be interested in this PR given your involvement in #11317 and #12034. Since it's a major change, I'd like to have as many eyes as possible looking at this.

tay1orjones

Thanks for this! I just took a cursory glance at the storybook and the elements preview. Storybook looks fine, but the icons aren't being rendered in the elements preview.

[object Undefined] should be a rendered preview of each icon

lewandom · 2023-09-26T18:28:15Z

@tay1orjones thanks for spotting! Looks like I was mistaken that the toString function is unused. I restored both toString and toSVG, should help fix the problem.

lewandom · 2023-09-26T20:04:39Z

With one more change, the icons now show up in the carbon-elements preview.

jharvey10 · 2023-09-29T13:30:15Z

packages/cli/src/commands/bundle/typescript.js

+function formatGlobals(string) {
+  const mappings = string.split(',').map((mapping) => {
+    return mapping.split('=');
+  });
+  return mappings.reduce(
+    (acc, [pkg, global]) => ({
+      ...acc,
+      [pkg]: global,
+    }),
+    {}
+  );
+}
+
+function formatDependenciesIntoGlobals(dependencies) {
+  return Object.keys(dependencies).reduce((acc, key) => {
+    const parts = key.split('/').map((identifier, i) => {
+      if (i === 0) {
+        return identifier.replace(/@/, '');
+      }
+      return identifier;
+    });
+
+    return {
+      ...acc,
+      [key]: pascalCase(parts.join(' ')),
+    };
+  }, {});
+}
+
+async function findPackageFolder(entrypoint) {
+  let packageFolder = entrypoint;
+
+  while (packageFolder !== '/' && path.dirname(packageFolder) !== '/') {
+    packageFolder = path.dirname(packageFolder);
+    const packageJsonPath = path.join(packageFolder, 'package.json');
+
+    if (await fs.pathExists(packageJsonPath)) {
+      break;
+    }
+  }
+
+  return packageFolder;
+}
+
+module.exports = bundle;


For these bottom three functions, what are your thoughts about exporting them from javascript.js and importing them here instead of duplicating them in this file.

@jdharvey-ibm sounds like a good idea, I can do this.

jharvey10 · 2023-09-29T15:04:36Z

packages/icon-helpers/tsconfig.json

Any thoughts on if this would be a good time to create a top-level tsconfig.base.json and extend from that for here and in @carbon/react?

@jdharvey-ibm I was basing on top of this comment for distinct tsconfig.json files: #12034 (comment). But I like the idea of using a top-level base config. I can certainly introduce it.

Ah! Not gonna lie I forgot about that comment, but I do still agree with it 😂 . The major one was definitely @carbon/react so with that in a good place, this might be a good time to treat as much of that config as makes sense as the top-level stuff, and have thinner configs for both carbon-react and now icons-react.

It's totally up to you if you want to handle that in this PR or if you'd prefer we do that in a separate one.

I think it's a good idea to handle it here, after all I did introduce these duplicating lines and agree it doesn't look pretty. I'll do this in coming days.

packages/react/src/components/FileUploader/Filename.tsx

packages/react/tasks/build.js

jharvey10 · 2023-09-29T15:12:20Z

packages/react/tasks/build.js

+      typescript({
+        noEmitOnError: true,
+        noForceEmit: true,
+        outputToFilesystem: false,
+        compilerOptions: {
+          emitDeclarationOnly: true,
+          rootDir,
+          outDir,
+        },
+      }),
+      babel(babelConfig),


Some of these typescript config values seems to exist across a few different files (perhaps in slightly different forms). Is it possible to cue off of tsconfig files for these instead? Or is this maybe a limitation of the rollup tooling that we can't easily do that type of thing?

@jdharvey-ibm yeah, this is problematic... We have (at least) 2 different kinds of TS configs:

The tsconfig.json files are mainly used in development environment, e.g.: by IDE, eslint, etc.

The inline rollup configs, which emit to build directories.

In this PR, I didn't quite think about the difference between group 1 and group 2 - I just based on whatever was there for each group. I guess unifying into one base config and then extending with options that absolutely must be different (e.g. rootDir/outDir) is the right way to go. I'll look at this together with addressing your other comment (#14714 (comment)).

lewandom · 2023-10-02T10:36:25Z

@jdharvey-ibm I think I addressed your comments where you asked for extracting common TypeScript configuration. For better or worse - cause now there's a separate subproject that holds that config + some utils. Let me know if that's an acceptable solution.

jharvey10

I think these changes look pretty awesome! I like the introduction of the typescript-config-carbon package and how it gets used by the other packages.

One question: Can we safely remove the typescript dependency from all of the other packages now? My thinking being that we could use this new typescript-config-carbon as the definitive source of a typescript version across the entire monorepo.

@tay1orjones Putting this on your radar to look over the new package, since you're way more familiar with the impacts of adding this on the repo as a whole.

lewandom · 2023-10-03T16:55:09Z

One question: Can we safely remove the typescript dependency from all of the other packages now? My thinking being that we could use this new typescript-config-carbon as the definitive source of a typescript version across the entire monorepo.

@jdharvey-ibm I think we can, given I've forgotten to add it into icons-react and the build still works ;) I'll make a PR commit to remove explicit dependency on typescript in @carbon/react.

metonym

Awesome work!

Does the package.json for @carbon/icons-react need to be updated to include the entry point for types?

I.e.,

"main": "lib/index.js",
"module": "es/index.js",
+ "types": "lib/index.d.ts",

lewandom · 2023-10-03T19:52:51Z

@metonym thanks for reviewing!

Does the package.json for @carbon/icons-react need to be updated to include the entry point for types?

That's a great question, that I don't know the answer for... So far everything works without it:

@carbon/react doesn't have types defined, yet it works fine when imported in TypeScript projects,
@carbon/icons-react doesn't have types defined in this PR, yet importing it in @carbon/react works (e.g. in Accordion component) and recursive use via @carbon/react/icons works too in my dev env.

Also, the types field is very poorly documented - I only found one place: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html, and there's no information in there what actually uses the types field, how this mechanism works overall, and whether there are alternatives (cause things apparently work without it).

I'm willing to have additional discussions on this, if you have more information. For now, I'd rather keep this as is, since it works, and @carbon/react doesn't define it either.

tay1orjones

Hey thanks for this! I think this looks fine overall. Extracting the typescript config into it's own package is a great enhancement 👍

tw15egan

LGTM, small merge conflict to resolve then we should be good to go. Thanks for making this enhancement!

metonym · 2023-10-24T19:19:45Z

These changes are published in @carbon/[email protected].

I'm testing the types, and they work great.

✅ import { WatsonHealth3DCursor } from "@carbon/icons-react";
✅ import { Accessibility } from "@carbon/icons-react/es";

However, I'm getting a type error when attempting to import using the full path.

For example:

❌ import Icon from "@carbon/icons-react/es/4K";

The type def looks like this:

// node_modules/@carbon/icons-react/es/4K.d.ts

import type { CarbonIconType } from './CarbonIcon';
export const _4K: CarbonIconType;

While the generated JS code (abridged) has a default export:

// node_modules/@carbon/icons-react/es/4K.js
// ...
export { _4K as default };

Should the typedef for an individual file instead have a default export?

- export const _4K: CarbonIconType;
+ declare const _4K: CarbonIconType;
+ export default _4K;

Can someone else confirm that they are able to repro? I hesitate to open a bug unless confirmed.

lewandom · 2023-10-26T11:54:33Z

@metonym thanks for finding this, I'll try to reproduce and address.

metonym · 2023-10-30T21:58:04Z

As of version 11.29.0, @types/carbon__icons-react has been removed from DefinitelyTyped.

I've also archived the source code used to generate the @types/carbon__icons-react code.

lewandom requested a review from a team as a code owner September 26, 2023 08:14

lewandom requested review from alisonjoseph and andreancardona September 26, 2023 08:14

tay1orjones requested review from tw15egan, tay1orjones and jharvey10 September 26, 2023 13:33

tay1orjones requested changes Sep 26, 2023

View reviewed changes

lewandom force-pushed the typescript-icons-react branch from 0e97640 to e769119 Compare September 26, 2023 18:26

lewandom requested a review from tay1orjones September 26, 2023 18:28

jharvey10 reviewed Sep 29, 2023

View reviewed changes

lewandom requested a review from jharvey10 October 3, 2023 07:28

jharvey10 reviewed Oct 3, 2023

View reviewed changes

lewandom force-pushed the typescript-icons-react branch from 0997f85 to 12c9248 Compare October 3, 2023 17:57

metonym reviewed Oct 3, 2023

View reviewed changes

metonym approved these changes Oct 3, 2023

View reviewed changes

alisonjoseph removed their request for review October 9, 2023 12:21

tay1orjones approved these changes Oct 18, 2023

View reviewed changes

tay1orjones added the status: one more review 👀 label Oct 18, 2023

tw15egan approved these changes Oct 18, 2023

View reviewed changes

lewandom force-pushed the typescript-icons-react branch from 12c9248 to d231524 Compare October 19, 2023 07:04

andreancardona added status: ready to merge 🎉 and removed status: one more review 👀 labels Oct 19, 2023

github-actions bot added this pull request to the merge queue Oct 19, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 19, 2023

tw15egan added this pull request to the merge queue Oct 19, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 19, 2023

tw15egan added status: merge conflict 🚫 and removed status: ready to merge 🎉 labels Oct 19, 2023

lewandom added 5 commits October 19, 2023 18:40

refactor(icons-react): add TypeScript types to icons-react package

aa73a0f

fix(FileUploader.Filename): restore tabIndex on complete icon

f85661f

fix(icon-helpers): restore icon-helpers exports

9477851

refactor(typescript): extract common TypeScript config

6355108

fix(typescript): single source of truth for TS version

b7cc8ce

lewandom force-pushed the typescript-icons-react branch from d231524 to b7cc8ce Compare October 19, 2023 16:56

Merge branch 'main' into typescript-icons-react

b9f88b4

tw15egan added status: ready to merge 🎉 and removed status: merge conflict 🚫 labels Oct 19, 2023

github-actions bot enabled auto-merge October 19, 2023 17:28

Merge branch 'main' into typescript-icons-react

29c685c

github-actions bot added this pull request to the merge queue Oct 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 20, 2023

tw15egan added this pull request to the merge queue Oct 20, 2023

Merged via the queue into carbon-design-system:main with commit 51d41fa Oct 20, 2023
14 checks passed

lewandom deleted the typescript-icons-react branch October 20, 2023 14:09

metonym mentioned this pull request Oct 24, 2023

feat(carbon__icons-react): not needed as of version 11.29.0 DefinitelyTyped/DefinitelyTyped#67176

Merged

9 tasks

lewandom mentioned this pull request Oct 26, 2023

fix(icons-react): fix exports in individual icon TypeScript declarations #15032

Merged

lewandom mentioned this pull request Nov 2, 2023

fix(react): publish type definitions for @carbon/react/icons #15086

Merged

cuppajoey mentioned this pull request Mar 14, 2024

[Bug]: CarbonIconType is not exported from @carbon/react/icons #15974

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(icons-react): add TypeScript types to icons-react package #14714

refactor(icons-react): add TypeScript types to icons-react package #14714

lewandom commented Sep 26, 2023 •

edited

Loading

netlify bot commented Sep 26, 2023 •

edited

Loading

netlify bot commented Sep 26, 2023 •

edited

Loading

lewandom commented Sep 26, 2023 •

edited

Loading

tay1orjones left a comment

lewandom commented Sep 26, 2023

lewandom commented Sep 26, 2023

jharvey10 Sep 29, 2023 •

edited

Loading

lewandom Sep 29, 2023

jharvey10 Sep 29, 2023

lewandom Sep 29, 2023

jharvey10 Sep 29, 2023

lewandom Sep 29, 2023

jharvey10 Sep 29, 2023

lewandom Sep 29, 2023

lewandom commented Oct 2, 2023

jharvey10 left a comment

lewandom commented Oct 3, 2023 •

edited

Loading

metonym left a comment

lewandom commented Oct 3, 2023

tay1orjones left a comment

tw15egan left a comment

metonym commented Oct 24, 2023 •

edited

Loading

lewandom commented Oct 26, 2023

metonym commented Oct 30, 2023 •

edited

Loading

refactor(icons-react): add TypeScript types to icons-react package #14714

refactor(icons-react): add TypeScript types to icons-react package #14714

Conversation

lewandom commented Sep 26, 2023 • edited Loading

Changelog

Testing / Reviewing

netlify bot commented Sep 26, 2023 • edited Loading

✅ Deploy Preview for v11-carbon-react ready!

netlify bot commented Sep 26, 2023 • edited Loading

✅ Deploy Preview for carbon-elements ready!

lewandom commented Sep 26, 2023 • edited Loading

tay1orjones left a comment

Choose a reason for hiding this comment

lewandom commented Sep 26, 2023

lewandom commented Sep 26, 2023

jharvey10 Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

lewandom Sep 29, 2023

Choose a reason for hiding this comment

jharvey10 Sep 29, 2023

Choose a reason for hiding this comment

lewandom Sep 29, 2023

Choose a reason for hiding this comment

jharvey10 Sep 29, 2023

Choose a reason for hiding this comment

lewandom Sep 29, 2023

Choose a reason for hiding this comment

jharvey10 Sep 29, 2023

Choose a reason for hiding this comment

lewandom Sep 29, 2023

Choose a reason for hiding this comment

lewandom commented Oct 2, 2023

jharvey10 left a comment

Choose a reason for hiding this comment

lewandom commented Oct 3, 2023 • edited Loading

metonym left a comment

Choose a reason for hiding this comment

lewandom commented Oct 3, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

tw15egan left a comment

Choose a reason for hiding this comment

metonym commented Oct 24, 2023 • edited Loading

lewandom commented Oct 26, 2023

metonym commented Oct 30, 2023 • edited Loading

lewandom commented Sep 26, 2023 •

edited

Loading

netlify bot commented Sep 26, 2023 •

edited

Loading

netlify bot commented Sep 26, 2023 •

edited

Loading

lewandom commented Sep 26, 2023 •

edited

Loading

jharvey10 Sep 29, 2023 •

edited

Loading

lewandom commented Oct 3, 2023 •

edited

Loading

metonym commented Oct 24, 2023 •

edited

Loading

metonym commented Oct 30, 2023 •

edited

Loading