Skip to content
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

Upgrade dev dependencies to reduce vulnerabilities #5840

Merged
merged 1 commit into from Nov 18, 2018

Conversation

simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Nov 15, 2018

Trying to reduce vulnerabilities reported in #5838:

Before:

found 113 vulnerabilities (26 low, 60 moderate, 27 high) in 10371 scanned packages

After:

found 76 vulnerabilities (5 low, 57 moderate, 14 high) in 15335 scanned packages

Remaining issues are from the gitbook package which doesn't seem maintained anymore so we will not be able to eliminate associated vulnerabilities (see /GitbookIO/gitbook-cli#87) unless we switch to another tool (e.g. vue-press)?!

Major upgrades:

  • browserify 14 -> 16
  • eslint 4 -> 5: new rules (locally disabled)
  • eslint-plugin-html 4 -> 5
  • gulp 3 -> 4: migrate gulp.js but I noticed that the latest version of gulp referenced on npm is 3.9.1 so I'm not sure if it's a good idea to upgrade. @etimberg @benmccann?
  • gulp-eslint 4 -> 5
  • gulp-replace 0 -> 1
  • gulp-size 2 -> 3
  • jasmine 2 -> 3
  • jasmine-core 2 -> 3
  • karma 1 -> 3
  • karma-jasmine 1 -> 2
  • karma-jasmine-html-reporter 0 -> 2
  • vinyl-source-stream 1 -> 2
  • yargs 9 -> 12

Upgrading karma and/or jasmine also highlighted issues with two of our unit tests.


// Check and see if tooltip was displayed
var tooltip = chart.tooltip;

expect(chart.lastActive).toEqual([point]);
expect(tooltip._lastActive).toEqual([]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evertvdw I updated this test which I think was wrong since the beginning (tooltip._lastActive should not be empty if the tooltip was correctly displayed). If you put a debugger here, you will notice that the tooltip is hidden (@etimberg).

@benmccann
Copy link
Contributor

GitBook has abandoned their open source tools and moved to a hosted solution. Their pricing page says "GitBook is free for open-source & non-profit teams". That may be the easiest solution since we're already setup to use GitBook

You can setup their hosted version to deploy from GitHub automatically. By default it publishes whatever is in master, which would probably not work for us since docs would get deployed for changes that haven't been released yet. But you can also set it up to use branches, so we could have a latest branch that we push to everytime we release a new version and want to have the docs deployed again

@simonbrunel
Copy link
Member Author

GitBook has abandoned their open source tools and moved to a hosted solution

Source?

That may be the easiest solution since we're already setup to use GitBook

It doesn't look like the easiest solution: we will have to break our CI to split the site / samples from the docs. We will not be able to map our docs under https://www.chartjs.org/docs, neither have versioning under https://www.chartjs.org/docs/2.7.2. That means we would need to change how versions work (or keep only the latest one, which is not great) and use a subdomain (docs.chartjs.org).

Also, I don't like the idea to base all our docs on tools fully hosted and have no way to generate the exact same result locally. I don't want to use their editor to update our markdown files in order to see the final render. What about if they shutdown their business tomorrow?

That's why I prefer to evaluate another tool such as VuePress (or similar) which would be easier to integrate than hosted GitBook. I migrated one of my project to VuePress (result) and it was really simple, almost no change in the markdown files. It's faster to generate, more stable on live reload, more flexible and it fits perfectly in our current CI process (I'm pretty sure we will have nothing to change on that part).

I still want @etimberg to review this PR, then let's merge, we don't have to figure out what to do for our docs right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants