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

Update (dev) dependencies, address github security alert #293

Merged
merged 6 commits into from Jun 11, 2019
Merged

Conversation

Macil
Copy link
Collaborator

@Macil Macil commented Jun 8, 2019

It seems like Github emailed a "security alert" to the maintainers of Kefir because of a dev-only subdependency. I'm pretty confident it doesn't really affect us or users of Kefir (it only matters if untrusted content gets passed to a subdependency that's only used during build), but it would be nice to see that alert go away, and it's also generally nice to have our dependencies up-to-date.

This PR is best viewed commit-by-commit. Changes to package-lock.json were done by npm (after any edits to package.json, I would remove package-lock.json and run npm install so npm regenerated it from scratch).

  • 83493ff I updated most dev dependencies. I switched most dev dependencies to being specified by major version (using ^) since they're pinned anyway by package-lock.json. I did not update prettier yet because that would require other files to be re-formatted, and I didn't update rollup because I'm not very familiar with it and I think updating it might mean updating to Babel 7 which is a bigger change than I want to do here. I did update Rxjs to version 6 which required a tiny change in one test. I believe this PR fixes the security alert by updating mocha and inquirer, which I think had the vulnerable subdependencies.
  • 0bc3df6 I changed symbol-observable from being listed as a regular dependency to being listed as a devDependency. When we build Kefir, we build symbol-observable directly into the Kefir bundle, so there's no need for the consumers of Kefir to install symbol-observable.
  • 75198fb I updated Prettier to the latest version, and let it reformat all files.
  • 793474c I moved the Prettier config out of prettier.sh into Prettier's .prettierrc config file. (Presumably editor plugins will be able to make use of that too.) I also fixed an issue in prettier.sh where it would fail to run if the path to the prettier executable had spaces.
  • 096d628 Updated the sinon dev dependency. I was worried this might require some changes in the tests so I didn't update this originally, but it turned out fine.

I diffed the built Kefir bundle after the first two commits (before I updated Prettier) to master. The only changes were minor ones caused by symbol-observable being updated.

This PR is mostly for internal benefits. The compiled output only changes in a minor way because of formatting changes and minor changes in symbol-observable (that I don't think change its behavior in a way important for Kefir users), so I don't think this PR alone warrants a new version being published to npm yet.

Copy link
Member

@rpominov rpominov left a comment

Choose a reason for hiding this comment

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

Great work!

@mAAdhaTTah
Copy link
Collaborator

Should we be bundling symbol-observable? Seems like that should be external, altho that might be a breaking change 😕.

Also, after we merge this, how do we feel about adding dependabot to this repo?

configs/prettier.sh Outdated Show resolved Hide resolved
@rpominov
Copy link
Member

rpominov commented Jun 8, 2019

Should we be bundling symbol-observable? Seems like that should be external, altho that might be a breaking change 😕.

In my opinion it's not important, either way is good.

Also, after we merge this, how do we feel about adding dependabot to this repo?

Our dependencies are mostly dev. So it's not as important to keep them up to date.

Also, when upgrading a regular dependency you can automatically verify it using tests. This will not work for dev. So someone will have to manually check all those PRs.

@mAAdhaTTah
Copy link
Collaborator

In my opinion it's not important, either way is good.

Ok, let's leave it for now, I'll file an issue to revisit this in 4.0 if / when we decide to do that.

Copy link
Collaborator

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

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

LGTM. Moving prettier to pkg.json#scripts can go in this PR or separate, whatever works for you @Macil.

@Macil
Copy link
Collaborator Author

Macil commented Jun 11, 2019

Thanks for the comments and making the issue about symbol-observable. I inlined the prettier stuff.

@Macil Macil merged commit bcf560f into master Jun 11, 2019
@Macil Macil deleted the deps branch June 11, 2019 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants