-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: working typings for Paragon, better types for <Icon> component #3016
feat: working typings for Paragon, better types for <Icon> component #3016
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3016 +/- ##
=======================================
Coverage 93.18% 93.18%
=======================================
Files 249 249
Lines 4342 4342
Branches 1036 1036
=======================================
Hits 4046 4046
Misses 292 292
Partials 4 4 ☔ View full report in Codecov by Sentry. |
@@ -4,6 +4,7 @@ | |||
"noImplicitAny": true, | |||
"allowJs": false, | |||
"rootDir": "src", | |||
"outDir": "dist" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixed a major issue. Without this outDir
setting, the build
script was running tsc
but tsc was putting its output .d.ts
files into ./node_modules/@edx/typescript-config/dist/
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great! Thank you so much for putting this together! I'm looking forward to pulling this down and testing it with an MFE!
www/tsconfig.json
Outdated
@@ -1,7 +1,7 @@ | |||
{ | |||
"extends": "../tsconfig.json", | |||
"compilerOptions": { | |||
"rootDir": "src", | |||
"rootDir": "../", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious as to what happened before this was changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change, I was getting errors like this when running ./node_modules/.bin/tsc --project www --noEmit
from the main paragon
root:
www/src/components/AutoToc.tsx:5:24 - error TS6059: File '/.../paragon/src/index.ts'
is not under 'rootDir' '/.../paragon/www/src'.
'rootDir' is expected to contain all source files.
The file is in the program because:
Imported via '~paragon-react' from file '/.../paragon/www/src/components/AutoToc.tsx'
However, it seems like the commit 688d212 resolved this in another way, by changing from index.ts
to index.d.ts
so this change to www/tsconfig.json
's rootDir
is no longer needed. I'll just leave it as it was.
c1ad897
to
fd07756
Compare
fd07756
to
f014498
Compare
I tested this out in I'm not sure if this is a "task failed successfully" moment or if this means there's more that needs to be explored here. |
@brian-smith-tcril That happens when you have two different versions of Also, once this is merged and installed via NPM, its deduplication will make sure that doesn't happen. But here you're joining together two separate sets of |
@bradenmacdonald thanks for clarifying! I wasn't sure what you meant by "weird conflicts with this package" in that section. After following that step everything seems to work as expected! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally with frontend-app-authn
, everything is working as expected!
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 22.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
This represents my version of the minimal changes to Paragon to start getting (just a few!) useful type definitions shipping with it.
Before this PR, I wasn't seeing any types for Paragon imports, either in MFE code that uses it nor even in the
www
subproject of this repo, which nominally uses TypeScript.After this PR, you still won't see many types, but TypeScript will at least know which component exist and which don't (it can check the imports). And a few components -
<Icon>
,<Chip>
,<ChipCarousel>
, and<Bubble>
will have types because they are written in TypeScript (or in the case ofIcon
because there's a.d.ts
file).I deliberately didn't go any further than that because I want to keep the changeset small enough that we can confidently review and merge it. From that point on, it will be much easier to incrementally add types to individual components in separate PRs.
This PR:
Creates a
src/index.d.ts
file that makes all of the top-level exports visible to TypeScript. Without this fix, types were not being found in repos that import Paragon by path, such as thewww
subproject. It will be necessary to manually keep this file in sync withsrc/index.js
. To make that as easy as possible, I made sure the files have a 1:1 correspondence line-by-line.Example:
www/src/components/IconsTable.tsx
<Icon>
has no types<Icon>
is type checkedAdds
"types": "dist/index.d.ts",
topackage.json
. Without this change, types were not being found in MFEs that import Paragon via NPM.Renames
/src/Icon/Icon.test.jsx
to be a.tsx
file so that the<Icon />
component tests will use/check TypeScript.Fixes incorrect types in
src/Icon/index.d.ts
. Now thatIcon.test.tsx
is enabled for type checking, we see that it contained type errors, because the.d.ts
file was incomplete:Adds type information to the
icons
sub-package. Now you get auto-completion and compile-time validation of the icon names. The new file,icons/es5/index.ts
is completely auto-generated so there's no need to maintain the types manually.Adds some compile-time (TypeScript-only) tests to
Icon.test.tsx
to avoid regressions.Renames a few
.jsx
files in the documentation site (www
subfolder) to.tsx
because otherwise the linter was complaining that~paragon-react
couldn't be found.This is because aThis may not be an issue anymore after I changed the approach from step 1 above, but it doesn't hurt :).jsx
file cannot import Paragon'ssrc/index.ts
anymore after the change in step 1.Fixed the
tsconfig.json
(see inline comment) and separated out the build vs. dev tsconfigs.How to test in an MFE
frontend-app-course-authoring
andparagon
are peer folders on your host.tutor dev run course-authoring npm install
to do this for course-authoring)package.json
and change the line"@types/react": "17.0.0",
to"@types/react": "../frontend-app-course-authoring/node_modules/@types/react",
(otherwise you'll get weird conflicts with this package). Substitute the appropriate folder name if you're using a different MFE.npm install
in theparagon
folder.tutor mounts add course-authoring:/Users/yourname/path/to/paragon:/openedx/paragon
(adjust this command as needed)package.json
change"@openedx/paragon": "^21.5.7",
to"@openedx/paragon": "file:../paragon",
tutor dev run course-authoring npm install --force
(warnings about "Conflicting peer dependency" are expected; ignore them).jsx
file in the MFE that has// @ts-check
on its first line (or add that line in!), and you'll see type checking. Note that<Icon>
should have correct typing, and other Paragon components like<Stack>
should not show any type errors.Example, showing the types appearing in VS Code for Course Authoring MFE's
ImportTagsWizard.jsx
:Next Steps
Once this PR is merged, I think we should follow up with additional PRs to add typing to individual components (e.g. one component per PR, or similar). That can be done either by adding a
.d.ts
file for the component, or by converting the component implementation to TypeScript. In either case, I would encourage people to follow the example in this PR and make sure that the tests for the component are also converted to TypeScript (or some other mechanism is used to check that the types are working).Once everything has a type (or close to it), the
index.js
file can be renamed toindex.ts
and theindex.d.ts
file can be deleted :)Deploy Preview
The code for this page was changed, but you should not be able to see any changes:
https://deploy-preview-3016--paragon-openedx.netlify.app/foundations/icons/
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist