Typings

[joharei]

Fixes #249.

I've set up TypeScript in the core-react icons and tokens libraries, and configured rollup to run the TypeScript compiler when building. I've added JSDocs to the components in order for the project to build successfully, and I've converted the tests to TypeScript to verify if all of the components and props work correctly. There is still code that I haven't annotated with JSDoc explicitly, where the types are inferred by the TypeScript compiler, but it looks OK to me.

I also added a pnpm script called lint:types, that runs the TypeScript compiler and fails if there are errors in either the definitions or the tests. This should be run automatically as part of #185. I noticed that a couple of the existing tests are failing though (mentioned in #302).

The new JSDocs duplicates the existing PropTypes. The JSDoc types can in some ways replace the need for PropTypes, but there is some prop validation in code that cannot be represented by types. In order to avoid maintaining duplicate types I'm thinking the PropTypes could be removed. What do you think?

Checklist:

• Set up TypeScript for core-react
• Write tests in TypeScript to verify that the usage is as expected
• Set up and write typings for the icons and tokens libraries. These are probably simple enough for the TypeScript compiler to figure out most of the types itself
• Figure out what to do with PropTypes and JSDoc duplication

A couple of useful links:

• Type Checking JavaScript Files
• Using JSDOC-Based TypeScript

[joharei]

Also, was I right in installing TypeScript and other dependencies in the core-react module only, or should I install it project-wide?

[mimarz]

@joharei Were you finished with this for us to have a look? Just wondering since there has been no activity and its still marked as draft and WIP 😅

[joharei]

Hi @mimarz!
I got caught up with another project that is not yet using the EDS libraries, but I hope I can get this finished soon 😊
Until then I would very much like your feedback on my work so far, and especially your thoughts on my question about replacing PropTypes.

[mimarz]

Hi @mimarz!
I got caught up with another project that is not yet using the EDS libraries, but I hope I can get this finished soon 😊
Until then I would very much like your feedback on my work so far, and especially your thoughts on my question about replacing PropTypes.

Neato! We'll try to have a look at it after Easter some time then :)

Either way, good job, looks like you have really dived deep into this :D

[joharei]

Hi @mimarz and @vnys!
I'm happy with this PR, and have marked it as ready. Please have a look 😊

[joharei]

Note that there are some // @ts-ignores here and there, mostly because TypeScript tries to pick up the PropTypes, but doesn't do a good job of it. In particular, PropTypes.oneOf()s are best represented as string literals in TypeScript eg., 'left' | 'right', but TypeScript just automatically converts it to string 😒 If anyone has a better solution, I'm all ears.

[joharei]

Hi @mimarz and @vnys,
Will you be able to have a look at this PR soon? I will be leaving Equinor in the middle of May, so I'd like to see this work finished before then 😉

[vnys]

Will you be able to have a look at this PR soon?

Yes, sorry it’s taking so long – we’ll have a look at it asap 😊

[mimarz]

Hi @mimarz and @vnys,
Will you be able to have a look at this PR soon? I will be leaving Equinor in the middle of May, so I'd like to see this work finished before then 😉

I had a brief look at the changes and this approach feels like it might be cumbersome to maintain documenation and types (since its duplicated in propTypes and on main function).

We recently introduced react-docgen to help us with making our prop tables for documentation.

In regards to this I saw that there is a .d.ts generator based of this, react-dts-generator. Have you tried this?

[joharei]

In regards to this I saw that there is a .d.ts generator based of this, react-dts-generator. Have you tried this?

I think I had a look at it, but discarded it because it looks like it isn't maintained, and is being used by very few people. So I went with the method that seems to be most popular out there (beside rewriting it all to TypeScript 😉). Won't JSDocs with types also remove the need for react-docgen? So instead of maintaining prop-types, you will only need to maintain the JSDocs, from which both types and documentation can be generated.

[mimarz]

In regards to this I saw that there is a .d.ts generator based of this, react-dts-generator. Have you tried this?

I think I had a look at it, but discarded it because it looks like it isn't maintained, and is being used by very few people. So I went with the method that seems to be most popular out there (beside rewriting it all to TypeScript 😉). Won't JSDocs with types also remove the need for react-docgen? So instead of maintaining prop-types, you will only need to maintain the JSDocs, from which both types and documentation can be generated.

Alright. Good to know you opnion on react-dts-generator, haven't looked to much at it ourselves, just that its there.

We like the "react-docgen" approach if we can call it that since it keeps it all in one place and types are derived from code, and not a "comment". JSDocs is also written above the component function which splits it apart from where the types are defined, so more prone to typos 🤔

[joharei]

JSDocs is also written above the component function

You could put the @typedef docs wherever you want e.g., where the prop-types are now.

which splits it apart from where the types are defined

What types? The prop-types? I'm thinking the JSDoc can replace the prop-types, so they don't need to be duplicated.

so more prone to typos

The JSDocs are checked by TypeScript at compile time, so typos in variable names or types will cause compile errors 😉 Your favorite editor should also pick up the TypeScript config in tsconfig.json and give you errors on the go and auto completion.

[mimarz]

What types? The prop-types? I'm thinking the JSDoc can replace the prop-types, so they don't need to be duplicated.

so more prone to typos

The JSDocs are checked by TypeScript at compile time, so typos in variable names or types will cause compile errors 😉 Your favorite editor should also pick up the TypeScript config in tsconfig.json and give you errors on the go and auto completion.

Do you mean these?

Icon.propTypes = {
  /** @ignore */
  className: PropTypes.string,
  // Title for svg if used semantically
  title: PropTypes.string,
  // Valid colors
  color: PropTypes.string,
  // Vertical spacing
  size: PropTypes.oneOf([16, 24, 32, 40, 48]),
  // Rotation
  rotation: PropTypes.oneOf([0, 90, 180, 270]),
  // Name
  name: PropTypes.string.isRequired,
}

Does that mean the Typescript compiler adds these + d.ts during compile from JSDocs annotations? This works for people not using Typescript and consuming the components?

Edit: I saw your comment in the descriptions say we can remove propTypes, but I just find it hard to believe 😂 Cool that you can do it this way

[joharei]

It doesn't add the actual runtime propTypes, but tools pick up the types from the .d.ts files and give you the same feedback, but in compile time. So Icon.d.ts becomes:

/**
 * @typedef {object} Props
 * @prop {keyof import("@equinor/eds-icons")} name
 * @prop {string} [title] Title for svg if used semantically
 * @prop {string} [color] Valid colors
 * @prop {16 | 24 | 32 | 40 | 48} [size] Vertical spacing
 * @prop {0 | 90 | 180 | 270} [rotation]
 */
export const Icon: React.ForwardRefExoticComponent<Props & React.SVGAttributes<SVGElement> & React.RefAttributes<any>>;
export type Props = {
    name: "layers_off" | "layers" | "more_horizontal" | "more_verticle" | "fullscreen_exit" | "fullscreen" | "verticle_split" | "view_agenda" | "view_array" | "view_carousel" | "view_column" | "view_day" | "view_list" | "view_module" | "view_quilt" | "view_stream" | "view_week" | "grid_off" | "grid_on" | "dashboard" | "maximize" | "minimize" | "reorder" | "toc" | "zoom_in" | "zoom_out" | "all_out" | "pan_tool" | "list" | "sort_by_alpha" | "tune" | "focus_center" | "compare" | "details" | "touch" | "change_history" | "track_changes" | "work" | "work_off" | "work_outline" | "sort" | "filter_list" | "select_all" | "unarchive" | "send" | "move_to_inbox" | "priority_low" | "priority_high" | "inbox" | "paste" | "file_copy" | "delete_multiple" | "cut" | "edit" | "copy" | "block" | "clear" | "archive" | "add_circle_outlined" | "add_circle_filled" | "add_box" | "add" | "save" | "report_off" | "remove_outlined" | "remove" | "report" | "reply_all" | "reply" | "undo" | "redo" | "refresh" | "loop" | "autorenew" | "search_in_page" | "search_find_replace" | "history" | "update" | "restore" | "restore_page" | "setting_backup_restore" | "search" | "searched_history" | "favorite_filled" | "favorite_outlined" | "star_filled" | "star_half" | "star_circle" | "star_outlined" | "bookmarks" | "bookmark_filled" | "bookmark_outlined" | "bookmark_collection" | "delete_to_trash" | "delete_forever" | "done" | "done_all" | "restore_from_trash" | "close_cricle_outlined" | "close" | "check_circle_outlined" | "check" | "checkbox" | "checkbox_outline" | "checkbox_indeterminate" | "radio_button_selected" | "radio_button_unselected" | "switch_off" | "switch_on" | "calendar_event" | "calendar_accept" | "calendar_reject" | "timer" | "timer_off" | "calendar" | "calendar_today" | "time" | "calendar_date_range" | "alarm" | "alarm_add" | "alarm_off" | "alarm_on" | "infinity" | "hourglass_empty" | "hourglass_full" | "share" | "skype" | "whats_app" | "facebook_messenger" | "microsoft_excel" | "microsoft_word" | "microsoft_powerpoint" | "github" | "spotify" | "youtube" | "youtube_alt" | "apple_app_store" | "twitter" | "apple_logo" | "instagram" | "facebook" | "chrome" | "ios_logo" | "linkedin" | "microsoft_edge" | "microsoft_onedrive" | "google_play" | "google_maps" | "microsoft_outlook" | "blocked_off" | "blocked" | "security" | "flagged_off" | "lock_add" | "lock_off" | "lock" | "lock_open" | "verified" | "verified_user" | "flagged" | "visibility" | "key" | "visibility_off" | "business" | "meeting_room" | "meeting_room_off" | "pool" | "cafe" | "gym" | "beach" | "world" | "school" | "city" | "account_circle" | "users_circle" | "face" | "group" | "group_add" | "person" | "person_add" | "baby" | "shopping_cart_add" | "shopping_basket" | "shopping_card" | "shopping_cart_off" | "credit_card" | "receipt" | "notifications_important" | "notifications_add" | "do_not_disturb" | "notifications" | "notifications_active" | "notifications_off" | "notifications_paused" | "warning_outlined" | "warning_filled" | "error_outlined" | "error_filled" | "sync" | "sync_off" | "sync_problem" | "menu" | "apps" | "home" | "exit_to_app" | "launch" | "open_in_browser" | "external_link" | "category" | "settings" | "van" | "motorcycle" | "transit_enter_exit" | "trip_origin" | "satellite" | "traffic_light" | "hospital" | "map" | "parking" | "directions" | "transfer" | "terrain" | "mall" | "ticket" | "pharmacy" | "cinema" | "convenience_store" | "car_wash" | "library" | "store" | "hotel" | "grocery_store" | "walk" | "run" | "bike" | "boat" | "place_unknown" | "flight" | "subway_tunnel" | "tram" | "train" | "shipping" | "taxi" | "transit" | "subway" | "car" | "railway" | "bus" | "departure_board" | "place_edit" | "place_add" | "place_person" | "pin_drop" | "place" | "view_360" | "gps_fixed" | "gps_not_fixed" | "gps_off" | "near_me" | "navigation" | "compass_calibration" | "flight_land" | "flight_takeoff" | "communte" | "explore" | "explore_off" | "toll" | "camera" | "phone" | "wifi" | "wifi_off" | "usb" | "bluetooth" | "bluetooth_connected" | "bluetooth_disabled" | "bluetooth_searching" | "apple_airplay" | "sim_card" | "scanner" | "router" | "memory" | "headseat_mic" | "headset" | "gamepad" | "speaker_group" | "speaker" | "mouse" | "keyboard_hide" | "keyboard" | "keyboard_voice" | "smartwatch" | "tv" | "tablet_ipad" | "tablet_android" | "iphone" | "android" | "dock" | "device_unknown" | "desktop_windwos" | "desktop_mac" | "computer" | "google_cast_connected" | "google_cast" | "dns" | "fingerprint_scanner" | "print_off" | "print" | "flash_off" | "flash_on" | "style" | "color_palette" | "dropper" | "texture" | "pram" | "fridge" | "breifcase" | "dice" | "toilet" | "nature_people" | "nature" | "snow" | "thermostat" | "money" | "dollar" | "flower" | "laundry" | "cake" | "cocktail" | "coffee" | "drink" | "pizza" | "fast_food" | "restaurant" | "dining" | "puzzle" | "placeholder_icon" | "smoking_off" | "smoking" | "widgets" | "puzzle_filled" | "bandage" | "brush" | "measure" | "report_bug" | "build_wrench" | "gavel" | "folder_shared" | "folder_add" | "folder_open" | "folder" | "file_description" | "file" | "folder_favorite" | "file_add" | "library_video" | "library_add" | "library_books" | "library_music" | "library_image" | "library_pdf" | "download" | "upload" | "download_done" | "cloud_upload" | "cloud_off" | "cloud_download" | "cloud_done" | "cloud" | "offline" | "offline_saved" | "collection_1" | "collection_2" | "collection_3" | "collection_4" | "collection_5" | "collection_6" | "collection_7" | "collection_8" | "collection_9" | "collection_9_plus" | "attachment" | "attach_file" | "image" | "movie_file" | "slideshow" | "image_add" | "sun" | "battery_alert" | "battery_charging" | "battery" | "battery_unknown" | "flame" | "waves" | "ev_station" | "gas_station" | "light" | "power_button" | "power_button_off" | "turbine" | "lightbulb" | "power" | "flare" | "link_off" | "link" | "camera_add_photo" | "code" | "gesture" | "text_rotation_up" | "text_rotation_verticle" | "text_rotation_angled_down" | "text_rotation_angled_up" | "text_rotation_down" | "text_rotation_none" | "opacity" | "invert_colors" | "flip_to_back" | "flip_to_front" | "insert_link" | "functions" | "format_stikethrough" | "wrap_text" | "verticle_align_top" | "verticle_align_center" | "verticle_align_bottom" | "title" | "text_field" | "format_shape" | "format_quote" | "format_list_numbered" | "format_list_bulleted" | "format_line_spacing" | "format_size" | "format_highlight" | "format_underline" | "format_italics" | "format_indent_increase" | "format_indent_decrease" | "format_color_text" | "format_color_reset" | "format_color_fill" | "format_clear" | "format_bold" | "format_align_right" | "format_align_left" | "format_align_justify" | "format_align_center" | "drag_handle" | "keyboard_space_bar" | "border_verticle" | "border_top" | "border_style" | "border_right" | "border_outer" | "border_left" | "border_inner" | "border_horizontal" | "border_color" | "border_clear" | "border_bottom" | "border_all" | "keyboard_backspace" | "keyboard_capslock" | "keyboard_return" | "keyboard_tab" | "rotate3d" | "spellcheck" | "rotate_90_degrees_ccw" | "rotate_left" | "flip" | "edit_text" | "crop_rotate" | "crop" | "rotate_right" | "video_chat" | "comment_important" | "comment_more" | "email_draft" | "share_screen_off" | "share_screen" | "unsubscribe" | "email" | "dialpad" | "contacts" | "contact_phone" | "contact_email" | "email_alpha" | "call_add" | "call_end" | "call" | "comment_discussion" | "comment_chat" | "comment_solid" | "comment" | "mood_very_happy" | "mood_very_sad" | "mood_sad" | "mood_neutral" | "mood_happy" | "mood_extreamly_sad" | "mood_extreamly_happy" | "comment_add" | "comment_chat_off" | "comment_notes" | "mail_unread" | "thumbs_down" | "thumbs_up" | "thumbs_up_down" | "support" | "help" | "help_outline" | "info_circle" | "donut_large" | "donut_outlined" | "table_chart" | "multiline_chart" | "pie_chart" | "bubble_chart" | "scatter_plot" | "bar_chart" | "assignment" | "assignment_user" | "assignment_important" | "assignment_return" | "assignment_returned" | "assignment_turned_in" | "timeline" | "trending_down" | "trending_flat" | "trending_up" | "eject" | "music_note" | "music_note_off" | "mic" | "mic_outlined" | "mic_off" | "volume_up" | "volume_off" | "volume_mute" | "volume_down" | "videocam_off" | "videocam" | "video_call" | "missed_video_call" | "playlist_play" | "playlist_added" | "playlist_add" | "snooze" | "shuffle" | "repeat_one" | "repeat" | "movie" | "res_hd_outlined" | "res_hd_filled" | "forward_30" | "forward_10" | "forward_5" | "replay_30" | "replay_10" | "replay_5" | "replay" | "play_circle_outlined" | "play_circle" | "play" | "pause_circle_outlined" | "pause_circle" | "pause" | "stop" | "record" | "skip_previous" | "skip_next" | "fast_rewind" | "fast_forward" | "closed_caption_outlined" | "closed_caption_filled" | "res_4k_outlined" | "res_4k_filled" | "record_voice" | "record_voice_off" | "subdirectory_arrow_right" | "subdirectory_arrow_left" | "last_page" | "first_page" | "unfold_more" | "unfold_less" | "arrow_drop_left" | "arrow_drop_right" | "arrow_drop_up" | "arrow_drop_down" | "arrow_forward_ios" | "arrow_back_ios" | "arrow_up" | "arrow_forward" | "arrow_down" | "arrow_back" | "chevron_down" | "chevron_left" | "chevron_right" | "chevron_up" | "swap_horizontal" | "swap_horizontal_circle" | "swap_verticle" | "swap_verticle_circle" | "label" | "label_off" | "tag" | "high_priority" | "new_label" | "new_alert" | "flag" | "pin" | "pregnant_woman" | "accessible" | "wheelchair" | "accessible_forward" | "language" | "google_translate" | "hearing" | "translate";
    /**
     * Title for svg if used semantically
     */
    title?: string;
    /**
     * Valid colors
     */
    color?: string;
    /**
     * Vertical spacing
     */
    size?: 16 | 24 | 32 | 40 | 48;
    rotation?: 0 | 90 | 180 | 270;
};
import React from "react";

Note that in this case you'll get suggestions for what icon name to send as a prop!

[ooystein]

This looks great! Seems like @joharei has put in a fair bit of time on this!

I tested a few components in both a Javascript and a TypeScript React project and the types worked as expected, giving IntelliSense in both projects and type checking in the TypeScript project.

I would highly recommend merging this and start using Typescript Types instead of the React propTypes. It gives better type hints/IntelliSense in the editor, is better for documentation, can give type checking in the projects own tests and in other Typescript projects. But as discussed, it will not give the runtime errors that some Javascript developers might expect from the propTypes.

The extra complexity this introduces to the project should be considered:

1. Bringing in Typescript dependencies
2. Maintaining Typescript configs
3. Extra compile step for generating type definition files
4. Switching test files to TS
5. Running tests on TS code (transpiling?)
6. Write JSDoc comments instead of propTypes

I can throw in my two cents on those issues:

1. The new dependencies are only devDependencies, and besides the type definition dependencies it's these three; typescript, tslib, rollup-plugin-typescript2 which are all basic and used in most typescript projects, so well supported and stable.
2. The configs are fairly small, rarely touched and all duplicates of each other.
3. The generation of the definition files seems fast and the setup is the same as I use in pure TS projects, so should be fairly well understood and stable.
4. This is a matter of taste. I would count it as a plus to work in TypeScript :P and embrace the extra type checking you get.
5. I was surprised to see that jest handled the switch to TS without any configuration changes to it so I don't see any issue here.
6. It might take some getting used to, but then again we could use the opportunity to write more documentation. As for remembering to write the JSDoc blocks I don't have any good solution there. Might be a lint rule for it that is reasonable.

I have not gone through all of the components in this PR so don't consider this a complete review. I also noticed a few code changes that I didn't immediately understand why was part of this PR and should be understood before merging.

If merged, one should follow up with deleting the propTypes and disable the eslint rule react/prop-types

If I got anything wrong or anyone notices anything important that I missed, please correct me or bring it up 👍

[ooystein]

If decided that propTypes are obsolete since the component will be typed with TSDoc, these ts-ignore will not be a problem either.

[ooystein]

Could consider defining these variant sum types if they are reused multiple times.

Example:

/**
 * @typedef ChipVariants
 * @type {('active' | 'error' | 'default')}
 */

[joharei]

Absolutely!

[ooystein]

I tried to look for a solution to these issues with sum types. But there doesn't seem to be a solution for this, https://stackoverflow.com/questions/56415416/typescript-jsdoc-group-sum-type

Defining the variants as a type first didn't help either.

The generated types are correct though, so type checking and code completion tools will pick it up in consuming projects.

[joharei]

defaultProps is actually going to be deprecated (see this PR and RFC). Maybe JS' default arguments could solve this problem?

[joharei]

Thanks @ooystein for the thorough feedback!

When it comes to no. 5, Jest doesn't actually run the TypeScript compiler, but I think it uses babel to transpile the code and run it, regardless of any compiler errors. This is the same in complete TypeScript projects as well. That's why I added the lint:types script so that compiler errors will be reported. I was thinking this script should be run along with the tests on whatever CI you're using.

Otherwise, I think you and I agree on these things 😉

[joharei]

I'd like to remind you that my last day in Equinor is on Friday. If you'd like my help to merge this PR, it will have to be before then.

[vnys]

@joharei We need more time to go through this, there are a lot of file changes 😅. Can you change the target branch from develop to typescript-jsdoc? Then we can merge it into that branch until we have the time to go through it properly.

[ooystein]

If you need any help with this now that @joharei is gone I'm happy to help.

[vnys]

If you need any help with this now that @joharei is gone I'm happy to help.

Thanks! I hope he still get’s notifications, if not we’ll have to merge in a local copy of his pr, but then we’ll probably lose this discussion.

[joharei]

I'll have a look at it one of the next days 👍

[joharei]

@vnys Changed the target branch.

[vnys]

@joharei Great, thank you! And thank you for your contribution to the Equinor Design System 😊

[vnys]

@ooystein Thank you for the thorough code review – we’ll probably need some more assistance when we’re ready to move forward with this, which I hope won’t be long.