-
Notifications
You must be signed in to change notification settings - Fork 2
Dependency notes #358
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
base: dev
Are you sure you want to change the base?
Dependency notes #358
Conversation
Formatted the information to be more presentable and fixed some mistakes in the writing.
Rtyujklop
left a comment
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.
These are the changes I can see now, overall it's good some tweaks can be done if you want.
README.md
Outdated
| ## Dependencies | ||
| ### root | ||
| `ajv`: seems to be unused, may be a subdependency. Compiles JSON schemas to JavaScript code. |
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.
Since ajv is flagged as “seems to be unused”, it might be worth explicitly noting whether it appears in dependencies vs devDependencies in package.json
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.
Changes from a recent commit now note which of the packages are used as devDependencies.
README.md
Outdated
| `@semantic-ui-react/css-patch`: patches semicolon issue with semantic (should have been fixed in an update of semanticUI?). | ||
| `@testing-library/jest-dom`: currently unused. |
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.
For @testing-library/jest-dom and @testing-library/user-event, it might be helpful to say “kept for future test expansion” or “candidate for removal” so it’s clear whether they’re intentionally retained.
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.
A notice to keep these dependencies for now, has since been included.
README.md
Outdated
| `html-entities`: is used for the same thing as `he`. | ||
| `prop-types`: a tool used for validating the data types of properties. |
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.
Maybe mention whether the project is using PropTypes consistently in new components, or if this is mainly for legacy components.
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've now added more information about the components in which prop-types is used.
| `html-to-text`: converts HTML into formatted text. Unclear if this needs to be in root. | ||
| ### server |
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 is a really helpful addition to the README – having dependency intent documented will make future upgrades and removals much safer.
Dependency documentation now notes which dependencies are in devDependencies. Additionally, the accuracy of the documentation for the package ajv has been improved.
…or-Project into dependency-notes
Finished incomplete sentence in the caniuse-lite documentation and noted that the package is a transitive dependency of react-scripts similar to ajv.
Marrufof
left a comment
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.
It nice to have these written out.
Created documentation for node dependencies in the project.