Templarian / MaterialDesign-React

Dist for Material Design Icons React Component for JS/TypeScript
https://materialdesignicons.com
MIT License
139 stars 20 forks source link

Fixed 70 vulnerabilities found by npm audit & fixed tests with coverage #39

Closed jhlav closed 4 years ago

jhlav commented 4 years ago

After running npm install, it reported 70 security vulnerabilities. 1 is critical, and 66 of them are high. I decided to spend the day working on this as @mdi/js and @mdi/react have been really useful libraries for many projects I work on. I made a separate branch for this since there were breaking changes in the nyc library. I also added a major version bump to 1.3.0 for this same reason. There were several other errors with npm run testWithCoverage, so I worked through those and later got it to work with all tests passing.

npm audit no longer reports any vulnerabilities after the changes in this PR. All tests succeed, too.

Please review PR #38 first. The first three commits in here could be merged from that one first, then this PR would have just the one commit to merge. Perhaps that one could be released as 1.2.2 and this one as 1.3.0, or just release 1.3.0, whatever you think is best, @Templarian.

image

jhlav@linux:~/Dev/MaterialDesign-React$ npm run testWithCoverage

> @mdi/react@1.3.0 testWithCoverage /home/jhlav/Dev/MaterialDesign-React
> nyc -r lcov -e .ts -x "*.spec.tsx?" mocha -r ts-node/register -r jsdom-global/register tests/*.spec.tsx && nyc report

  <Icon path={path} /> Renders
    ✓ renders the the svg > path

  <Icon path={path} />
    ✓ verify svg[viewBox]
    ✓ verify svg.style (width='', transform='', animation='', transformOrigin='')
    ✓ verify svg > path[d]
    ✓ verify svg > path.style.fill

  <Icon path={path} size />
    ✓ verify size: number
    ✓ verify size: string

  <Icon path={path} horizontal />
    ✓ verify horizontal: boolean
    ✓ verify horizontal: boolean = true
    ✓ verify horizontal: boolean = false

  <Icon path={path} vertical />
    ✓ verify vertical: boolean
    ✓ verify vertical: boolean = true
    ✓ verify vertical: boolean = false

  <Icon path={path} rotate />
    ✓ verify rotate: number = 90

  <Icon path={path} color />
    ✓ verify color: string = '#000'

  <Icon path={path} spin />
    ✓ verify spin creates <style/>
    ✓ verify spin: boolean
    ✓ verify spin: boolean = {true}
    ✓ verify spin: boolean = {false}
    ✓ verify spin: number = {3}

  <Icon path={path} horizontal vertical rotate={90} />
    ✓ verify horizontal + vertical + rotate: boolean

  <Icon path={path} className={'foo'} />
    ✓ verify additional props

  <Icon /> A11y
    ✓ no title sets role=presentation
    ✓ title does not role=presentation
    ✓ title sets aria-labelledby
    ✓ title and description sets aria-labelledby
    ✓ title sets title
    ✓ title and description sets title and desc
    ✓ just description without title throws error

  <Stack><Icon path={path} /></Stack> Renders
    ✓ verify default props

  <Stack><Icon path={path} /></Stack> Inherited Props
    ✓ verify inherited props

  <Stack><Icon path={path} /></Stack> Size
    ✓ verify dom has size

  <Stack><Icon path={path} /></Stack> Props
    ✓ verify additional props

  <Stack /> A11y
    ✓ no title sets role=presentation
    ✓ title does not role=presentation
    ✓ title sets aria-labelledby
    ✓ title and description sets aria-labelledby
    ✓ title sets title
    ✓ title and description sets title and desc

  39 passing (74ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |                   
----------|---------|----------|---------|---------|-------------------
Templarian commented 4 years ago

@jhlav Those vulnerabilities are in the dev dependencies not the actual NPM package. 😉

Updating the deprecated SFC is a good catch, but might be a moment until I can fully scan this PR. Due to the heavy use of this component I need to be very careful with breaking changes.

Templarian commented 4 years ago

Closing this one out as it's safe to ignore the dev dependency vulnerabilities. It's down to 27 with update libraries.

In the future please break each individual PR up into a single feature. It makes reviewing PR's a lot easier.

Thanks for the work into both of these PRs. Leaving the onClick issue opened for discussion. 👍

jhlav commented 4 years ago

In the future please break each individual PR up into a single feature. It makes reviewing PR's a lot easier.

Will do. I haven't made many contributions like this so have some learning to do on how this process works, or how to do it better. I appreciate the feedback. :+1:

Thanks for the work into both of these PRs. Leaving the onClick issue opened for discussion.

You're welcome. Happy to help!