[BD-46] feat: add typescript support #1267

viktorrusakov · 2022-05-05T13:15:23Z

Description

Add TypeScript support

Deploy Preview

Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

netlify · 2022-05-05T13:15:30Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`5e46b37`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/62ebc022cab391000872fc42
😎 Deploy Preview	https://deploy-preview-1267--paragon-openedx.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.

openedx-webhooks · 2022-05-05T13:15:30Z

Thanks for the pull request, @viktorrusakov!

When this pull request is ready, tag your edX technical lead.

monteri · 2022-05-11T14:16:23Z

src/Bubble/index.tsx

-  'error',
-  'warning',
-];
+const STYLE_VARIANTS = ['primary', 'success', 'error', 'warning'] as const;


Is it good idea to change to enum?

enum STYLE_VARIANTS { PRIMARY = 'primary', SUCCESS = 'success', ... }

and then in the interface we can

... variant?: STYLE_VARIANTS, ...

I agree, an enum is cleaner for the types, but STYLE_VARIANTS is still used in the prop types. That said, once a component has types for the props as Bubble now does, the PropType definitions become less meaningful. They'd really only used for the purpose of forming the "Props API" tables on the component pages in the documentation site, I believe.

Will we need to keep both types and prop types moving forward or I wonder if we should do some technical discovery around migrating away from prop types while keeping the same functionality with generating the props api tables.

Yeah, I didn't use enum because it conflicts with propTypes, I didn't find a good way to reuse it in propTypes definition, so that it correctly displays in Props API table...

I think we will need to keep propTypes even when we move to TypeScript types, because they have different purpose - TypeScript types work only during compile type while propTypes during runtime, this means if we remove propTypes and consumers pass some variant that is not in STYLE_VARIANTS they will not get a warning that this prop is invalid. I guess they would if they have TypeScript setup inn their projects... but I believe most MFEs (if not all of them) do not use TypeScript

I guess they would if they have TypeScript setup inn their projects... but I believe most MFEs (if not all of them) do not use TypeScript

This is correct. The TypeScript in this PR is the first TypeScript introduced in the Open edX platform, AFAIK. It's a great callout for build time vs. runtime validation in TypeScript and propTypes; I agree it makes sense we should probably use both for the time being, until we learn otherwise.

adamstankiewicz

Looking good! Left a couple clarifying questions.

adamstankiewicz · 2022-05-13T11:34:42Z

src/Bubble/index.tsx

-  'error',
-  'warning',
-];
+const STYLE_VARIANTS = ['primary', 'success', 'error', 'warning'] as const;


I agree, an enum is cleaner for the types, but STYLE_VARIANTS is still used in the prop types. That said, once a component has types for the props as Bubble now does, the PropType definitions become less meaningful. They'd really only used for the purpose of forming the "Props API" tables on the component pages in the documentation site, I believe.

Will we need to keep both types and prop types moving forward or I wonder if we should do some technical discovery around migrating away from prop types while keeping the same functionality with generating the props api tables.

adamstankiewicz · 2022-05-13T11:35:18Z

tsconfig.json

+  "include": [
+    "src"
+  ]
+}


nit: newline

adamstankiewicz · 2022-05-13T11:35:33Z

src/withDeprecatedProps.tsx

        </WrappedComponent>
      );
    }
  }

+  // @ts-ignore


[curious] why does this need to be ts-ignore'd

TypeScript complained that displayName property does not exist in the component, so you can't assign it.

I force-pushed, so you can't see anymore, but ts-ignore complained about line

WithDeprecatedProps.displayName = `withDeprecatedProps(${componentName})`;

codecov · 2022-07-10T08:20:02Z

Codecov Report

Merging #1267 (5e46b37) into master (a1b7bf9) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1267      +/-   ##
==========================================
+ Coverage   91.42%   91.44%   +0.02%     
==========================================
  Files         205      205              
  Lines        3474     3484      +10     
  Branches      817      818       +1     
==========================================
+ Hits         3176     3186      +10     
+ Misses        284      283       -1     
- Partials       14       15       +1

Impacted Files	Coverage Δ
src/Button/deprecated/index.jsx	`92.00% <ø> (ø)`
src/CheckBox/index.jsx	`90.00% <ø> (ø)`
src/Chip/index.jsx	`90.90% <ø> (ø)`
...rc/DataTable/filters/MultiSelectDropdownFilter.jsx	`100.00% <ø> (ø)`
src/Dropdown/deprecated/index.jsx	`94.84% <ø> (ø)`
src/Hyperlink/index.jsx	`73.91% <ø> (ø)`
src/Icon/index.jsx	`100.00% <ø> (ø)`
src/InputSelect/index.jsx	`12.50% <ø> (ø)`
src/InputText/index.jsx	`88.88% <ø> (ø)`
src/MailtoLink/index.jsx	`100.00% <ø> (ø)`
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

viktorrusakov · 2022-07-12T06:57:06Z

Makefile

 	# --copy-files will bring in everything else that wasn't processed by babel. Remove what we don't want.
-	rm -rf dist/**/*.test.jsx


This didn't work as expected I think, build still had tests files inside tests folder (e.g. Datatable/tests/ is still in the dist directory with tests files in there), I think it deleted files only one level deep not going into all subdirectories.

Rewrote this to make sure we delete all tests and md files from the build

adamstankiewicz

This PR looks good to me! 🚀 I added just one comment about a potential follow-on ADR/documentation task for TypeScript, but let's get this PR in first.

Likely due to the recently merged security vulnerability package upgrades PR, there's bound to be quite a few merge conflicts on this PR unfortunately, though 😬

adamstankiewicz · 2022-07-29T11:10:02Z

package.json

+    "type-check": "tsc --noEmit",
+    "type-check:watch": "npm run type-check -- --watch",
+    "build-types": "tsc --emitDeclarationOnly"


It might be a good idea to add documentation about what these commands do and when/why we should use them somewhere in the repo. It very well could be the starting documentation for how to work with TypeScript for all of Open edX.

Related, did we write an ADR (Architectural Decision Record) about TypeScript? If not, that's probably something we should do. Is this something you might want to take on? Let's treat that as a task separate from this PR, though 😄

viktorrusakov · 2022-08-04T14:21:48Z

@adamstankiewicz rebased, should be good to go!

openedx-webhooks · 2022-08-05T12:32:42Z

@viktorrusakov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

# [20.8.0](v20.7.0...v20.8.0) (2022-08-05) ### Bug Fixes * fixed Card import ([#1522](#1522)) ([ebcf090](ebcf090)) ### Features * add typescript support ([#1267](#1267)) ([a31591e](a31591e))

edx-semantic-release · 2022-08-05T12:43:19Z

🎉 This PR is included in version 20.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels May 5, 2022

viktorrusakov marked this pull request as draft May 5, 2022 13:15

monteri reviewed May 11, 2022

View reviewed changes

adamstankiewicz reviewed May 13, 2022

View reviewed changes

adamstankiewicz mentioned this pull request May 13, 2022

Raccoon Gang (5/13) #1285

Closed

27 tasks

natabene assigned adamstankiewicz and unassigned adamstankiewicz May 14, 2022

viktorrusakov force-pushed the rusakov/add-typescript-support branch 2 times, most recently from 8132b7e to c2c4f11 Compare May 18, 2022 13:48

adamstankiewicz added the raccoon-gang label May 27, 2022

adamstankiewicz linked an issue May 27, 2022 that may be closed by this pull request

TypeScript foundations #1320

Closed

adamstankiewicz mentioned this pull request May 27, 2022

Raccoon Gang (5/27) #1324

Closed

viktorrusakov force-pushed the rusakov/add-typescript-support branch from c2c4f11 to b21ad01 Compare July 10, 2022 08:11

openedx-webhooks removed the needs triage label Jul 10, 2022

viktorrusakov marked this pull request as ready for review July 11, 2022 05:22

viktorrusakov changed the title ~~DRAFT: [BD-46] feat: add typescript support~~ [BD-46] feat: add typescript support Jul 11, 2022

viktorrusakov force-pushed the rusakov/add-typescript-support branch 2 times, most recently from 20ea335 to d002219 Compare July 11, 2022 09:28

viktorrusakov commented Jul 12, 2022

View reviewed changes

adamstankiewicz approved these changes Jul 29, 2022

View reviewed changes

feat: add typescript support

5e46b37

viktorrusakov force-pushed the rusakov/add-typescript-support branch from d002219 to 5e46b37 Compare August 4, 2022 12:48

adamstankiewicz merged commit a31591e into openedx:master Aug 5, 2022

edx-semantic-release added the released label Aug 5, 2022

adamstankiewicz added this to the TypeScript Support milestone Aug 12, 2022

adamstankiewicz removed a link to an issue Aug 12, 2022

TypeScript foundations #1320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-46] feat: add typescript support #1267

[BD-46] feat: add typescript support #1267

viktorrusakov commented May 5, 2022

netlify bot commented May 5, 2022 •

edited

Loading

openedx-webhooks commented May 5, 2022 •

edited

Loading

monteri May 11, 2022 •

edited

Loading

adamstankiewicz May 13, 2022

viktorrusakov Jul 11, 2022

adamstankiewicz Jul 29, 2022

adamstankiewicz left a comment

adamstankiewicz May 13, 2022

adamstankiewicz May 13, 2022

adamstankiewicz May 13, 2022

viktorrusakov Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 10, 2022 •

edited

Loading

viktorrusakov Jul 12, 2022

adamstankiewicz left a comment

adamstankiewicz Jul 29, 2022

viktorrusakov commented Aug 4, 2022

openedx-webhooks commented Aug 5, 2022

edx-semantic-release commented Aug 5, 2022

		# --copy-files will bring in everything else that wasn't processed by babel. Remove what we don't want.
		rm -rf dist/*/.test.jsx

[BD-46] feat: add typescript support #1267

[BD-46] feat: add typescript support #1267

Conversation

viktorrusakov commented May 5, 2022

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

netlify bot commented May 5, 2022 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

openedx-webhooks commented May 5, 2022 • edited Loading

monteri May 11, 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

adamstankiewicz 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

viktorrusakov Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorrusakov commented Aug 4, 2022

openedx-webhooks commented Aug 5, 2022

edx-semantic-release commented Aug 5, 2022

netlify bot commented May 5, 2022 •

edited

Loading

openedx-webhooks commented May 5, 2022 •

edited

Loading

monteri May 11, 2022 •

edited

Loading

viktorrusakov Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 10, 2022 •

edited

Loading