Skip to content

Update dependencies #88

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

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Update dependencies #88

merged 2 commits into from
Oct 15, 2019

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Oct 1, 2019

This updates the project's dependencies to the latest.

I have put this PR through some testing but I can't say for sure that this won't break anything.

Testing

Stolen from @dmsnell: Testing is a bit tricky but I found it easiest to link this library with the
Simplenote electron app and build them together via npm link.

git clone …node-simperium.git
git clone …simplenote-electron.git
cd node-simperium
npm checkout update/dependencies
npm install
npm run prepare
npm link
cd ..
cd simplenote-electron
npm link simperium
npm install
npm run dev

@belcherj belcherj self-assigned this Oct 1, 2019
@belcherj belcherj requested a review from dmsnell October 1, 2019 16:49
@belcherj belcherj force-pushed the update/dependencies branch from f657c0d to d4664d5 Compare October 1, 2019 16:51
@beaucollins
Copy link
Contributor

beaucollins commented Oct 1, 2019

The CI build failures seem to indicate something wrong with package-lock.json:

Unexpected token < in JSON at position 66446

Yeah, the package-lock.json changes contain a unresolved merge block.

Testing is a bit tricky but I found it easiest to link this library with the
Simplenote electron app and build them together via npm link.

Given the unit test coverage of the library at this point, for a development dependency update like this I would dare say npm test is good enough of a test plan.

Copy link
Contributor

@beaucollins beaucollins left a comment

Choose a reason for hiding this comment

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

Need that package-lock.json sorted out.

"dev": true,
"requires": {
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge issues here.

I think the correct remedy is:

git checkout origin/master -- package-lock.json
npm install
git add package-lock.json
git commit -m "Updated package-lock.json"
git push

@dmsnell
Copy link
Contributor

dmsnell commented Oct 1, 2019

Typically when I update package.json I will flush and reinstall my packages to get a new valid lock.

npm install webpack
git commit package.json
rm -rf node_modules
npm install
git commit package-lock.json

Inter-branch conflicts can also appear, which is part of why I recreate the lock from a fresh set of packages each time I change it.

@beaucollins
Copy link
Contributor

beaucollins commented Oct 1, 2019

The new package lock should also fix #89

I'm not sure how package-lock.json works with npm packages. Looking at our dependencies we should make websocket an optional dependency since browsers don't need it. And maybe @babel/polyfill should be a peer dependency?

I think the only hard dependency should be uuid. And I bet we could inline our own and become dependency-less.

@beaucollins
Copy link
Contributor

Discovered today when running npm install while I had a merge conflict on package-lock.json:

npm WARN conflict A git conflict was detected in package-lock.json. Attempting to auto-resolve.
npm WARN conflict To make this happen automatically on git rebase/merge, consider using the npm-merge-driver:
npm WARN conflict $ npx npm-merge-driver install -g

https://www.npmjs.com/package/npm-merge-driver

@dmsnell
Copy link
Contributor

dmsnell commented Oct 2, 2019

It seems like manually rebasing a branch before merging/squash-and-merging can uncover these. It's easy to get package-lock out of sync. Blame loose contracts/interfaces that are hallmark of the npm world

@belcherj
Copy link
Contributor Author

This is ready to go again. Sorry for the rebase issue.

Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

With npm test I see all 70 tests passing as well

@belcherj belcherj merged commit d069594 into master Oct 15, 2019
@belcherj belcherj deleted the update/dependencies branch October 15, 2019 03:39
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.

3 participants