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

chore(carbon__icons-react): ship TypeScript definitions #12034

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -219,3 +219,25 @@ jobs:
with:
name: playwright-report
path: .playwright

typecheck:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Use Node.js 16.x
uses: actions/setup-node@v3
with:
node-version: '16.x'
- uses: actions/cache@v3
id: cache
with:
path: |
node_modules
*/**/node_modules
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
- name: Install dependencies
run: yarn install --immutable --immutable-cache
- name: Build project
run: yarn build --ignore '@carbon/sketch'
- name: Typecheck TypeScript files
run: yarn typecheck
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,6 @@ package-lock.json

# Playwright
.playwright

# TypeScript
tsconfig.tsbuildinfo
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"sync": "carbon-cli sync",
"test": "cross-env BABEL_ENV=test jest",
"test:e2e": "cross-env BABEL_ENV=test jest -c jest.e2e.config.js",
"typecheck": "tsc -b",
Copy link
Contributor

Choose a reason for hiding this comment

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

This script name implies type-checking and not necessarily building. Would it make more sense for this to be tsc --noEmit?

"postinstall": "husky install"
},
"resolutions": {
Expand Down Expand Up @@ -73,7 +74,8 @@
"react": "^17.0.2",
"react-dom": "^17.0.2",
"rimraf": "^3.0.0",
"stylelint": "^14.3.0"
"stylelint": "^14.3.0",
"typescript": "^4.7.2"
},
"commitlint": {
"extends": [
Expand Down
12 changes: 12 additions & 0 deletions packages/icon-build-helpers/src/builders/react/next.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,18 @@ async function builder(metadata, { output }) {
exports: 'auto',
});
}

let declarationFile = `${templates.banner}

import { Icon } from './types/Icon';

`;

for (const m of modules) {
declarationFile += `export const ${m.name}: Icon;\n`;
}

await fs.writeFile(path.join(output, 'index.d.ts'), declarationFile);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/icons-react/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
# TODO: remove in v11
/next
# Generated declaration file
index.d.ts
7 changes: 5 additions & 2 deletions packages/icons-react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
"bugs": "https://github.com/carbon-design-system/carbon/issues",
"files": [
"es",
"lib"
"lib",
"index.d.ts",
"types"
],
"keywords": [
"ibm",
Expand All @@ -29,9 +31,10 @@
},
"scripts": {
"build": "yarn clean && node tasks/build.js",
"clean": "rimraf es lib",
"clean": "rimraf es lib index.d.ts",
"postinstall": "carbon-telemetry collect --install"
},
"types": "./index.d.ts",
"peerDependencies": {
"react": ">=16"
},
Expand Down
4 changes: 4 additions & 0 deletions packages/icons-react/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": "../../tsconfig.base.json",
"include": ["index.d.ts", "types"]
}
41 changes: 41 additions & 0 deletions packages/icons-react/types/Icon.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright IBM Corp. 2019, 2019
*
* This source code is licensed under the Apache-2.0 license found in the
* LICENSE file in the root directory of this source tree.
*/

import * as React from 'react';

type IconSize = 16 | 20 | 24 | 32 | '16' | '20' | '24' | '32' | number | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an outstanding comment relating to this that I think needs some discussion. You can ignore my comment on it because the answer to what I asked over there is "yes", but the comment about IDE autocompletion is worth exploring further.

Also, I'm not sure why strings are allowable on this prop. @tay1orjones Do react icons allow you to specify string versions of the number sizes?

Copy link
Member

Choose a reason for hiding this comment

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

@jdharvey-ibm Yes, both strings or number literals are allowed


// Inspired by the original types defined in DefinitelyTyped:
// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/31fb0ded56cff23b7e6693bd673b00f41dc00bbe/types/carbon__icons-react/index.d.ts
interface IconProps
extends Omit<
React.SVGProps<React.ReactSVGElement>,
'ref' | 'tabIndex' | 'aria-hidden'
> {
/**
* @default "currentColor"
*/
fill: string;

/**
* Specify the size of the icon. This value can be one of the predetermined
* sizes for this icon (16, 20, 24, 32) or it can be a custom value that is
* used for the width and height of the element
*
* @default 16
*/
size?: IconSize;

/**
* @default "http://www.w3.org/2000/svg"
*/
xmlns?: string;
}

export type Icon = React.ForwardRefExoticComponent<
IconProps & React.RefAttributes<SVGSVGElement>
>;
26 changes: 26 additions & 0 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"compilerOptions": {
"lib": ["es2015.iterable", "es2015.generator", "es5"],
"moduleResolution": "node",

"declaration": true,
"declarationMap": true,
"sourceMap": true,
"composite": true,
"noEmitOnError": true,

"strictNullChecks": true,
"noImplicitAny": true,
"noImplicitThis": true,
"strictPropertyInitialization": true,
"noUnusedLocals": true,
"noUnusedParameters": true,

"alwaysStrict": true,
"preserveConstEnums": true,

"jsx": "preserve",

"types": []
}
}
Comment on lines +1 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the PR in which we want to define the overall monorepo's tsconfig file. I'd be more comfortable with each respective package defining their own for now until we have a global baseline that is defined by the TS working group.

Separately, It might be worth generating the basic tsconfig files using the tsc --init command since it outputs all (read: most) possible settings and gives nice context to each of them. Then from there, we can make changes to the configs above and beyond the defaults as needed.

9 changes: 9 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"files": [],
"include": [],
"references": [
{
"path": "./packages/icons-react"
}
]
}
17 changes: 9 additions & 8 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -10796,6 +10796,7 @@ __metadata:
react-dom: ^17.0.2
rimraf: ^3.0.0
stylelint: ^14.3.0
typescript: ^4.7.2
languageName: unknown
linkType: soft

Expand Down Expand Up @@ -30908,23 +30909,23 @@ resolve@^2.0.0-next.3:
languageName: node
linkType: hard

"typescript@npm:^4.6.4":
version: 4.7.2
resolution: "typescript@npm:4.7.2"
"typescript@npm:^4.6.4, typescript@npm:^4.7.2":
version: 4.8.2
resolution: "typescript@npm:4.8.2"
bin:
tsc: bin/tsc
tsserver: bin/tsserver
checksum: 5163585e6b56410f77d5483b698d9489bbee8902c99029eb70cf6d21525a186530ce19a00951af84eefd4a131cc51d0959f5118e25e70ab61f45ac4057dbd1ef
checksum: 7f5b81d0d558c9067f952c7af52ab7f19c2e70a916817929e4a5b256c93990bf3178eccb1ac8a850bc75df35f6781b6f4cb3370ce20d8b1ded92ed462348f628
languageName: node
linkType: hard

"typescript@patch:typescript@^4.6.4#~builtin<compat/typescript>":
version: 4.7.2
resolution: "typescript@patch:typescript@npm%3A4.7.2#~builtin<compat/typescript>::version=4.7.2&hash=7ad353"
"typescript@patch:typescript@^4.6.4#~builtin<compat/typescript>, typescript@patch:typescript@^4.7.2#~builtin<compat/typescript>":
version: 4.8.2
resolution: "typescript@patch:typescript@npm%3A4.8.2#~builtin<compat/typescript>::version=4.8.2&hash=7ad353"
bin:
tsc: bin/tsc
tsserver: bin/tsserver
checksum: 7e2b9a9f4a70fb7616f1b0d986977f8e34a74f046202fa7f24fdee79589598277810fa216b3776c20c0683a9235872c73be34fdb93f67f98c1efaca40999422f
checksum: 6f49363af8af2fe480da1d5fa68712644438785208b06690a3cbe5e7365fd652c3a0f1e587bc8684d78fb69de3dde4de185c0bad7bb4f3664ddfc813ce8caad6
languageName: node
linkType: hard

Expand Down