-
Notifications
You must be signed in to change notification settings - Fork 348
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
Specify types location in package.json #973
Conversation
Size Change: +426 B (0%) Total Size: 815 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #973 +/- ##
==========================================
+ Coverage 63.51% 66.66% +3.14%
==========================================
Files 421 428 +7
Lines 96123 96944 +821
Branches 6253 9827 +3574
==========================================
+ Hits 61049 64624 +3575
+ Misses 35074 32320 -2754 see 88 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (7caa134) and published it to npm. You Example: yarn add @khanacademy/perseus@PR973 |
"files": [ | ||
"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.
why remove this? I think removing it means that the installed package will include extraneous files
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 moving this PR to a Draft. I was playing with figuring out why we're having Typescript issues in webapp and I haven't figured it out completely yet.
Ben fixed them with this PR (#971), but I don't feel like we truly understood why Typescript was seeing the files in the src/
dir of a node module.
I'm going to abandon this for now. We've moved to using an |
Summary:
NOTE: This PR is not in a state to land. I'm trying to figure things out, but this is not 100% yet.
Matthew ran into an issue while deploying a minor Perseus update today. It appears this is related to the fact that we were shipping source code, but we also suspect that it's because we haven't specified a
types
key in ourpackage.json
files.Issue: "none"
Test plan:
I'm going to take this PR's snapshot npm package and install it in webapp and see if the type issues remain.
Note that I've added a commit on this PR that re-enables shipping the
src
dir to see if we can reproduce the problem and if this change fixes it.