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

[91] update packages to resolve deep dependencies #92

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Conversation

justincorrigible
Copy link
Member

@justincorrigible justincorrigible commented Nov 5, 2020

Closes #91, supersedes #90, #85 and #93.

@@ -97,7 +97,7 @@ $ mongod
npm start
```

4. personal is now running and you can access it `http://localhost:3232/graphql`
4. persona is now running and you can access it `http://localhost:3232/graphql`
Copy link
Member Author

Choose a reason for hiding this comment

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

typo

package-lock.json Outdated Show resolved Hide resolved
@@ -43,7 +43,8 @@ const connect = () =>
reject(err);
} else {
console.log(`Connected to mongo at mongodb://${mongoHost}/${mongoDb}`);
resolve();
resolve(null); // null required by 'strictNullChecks'
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary in the latest version of TypeScript.

Looks like the maintainers walked themselves into a corner... still trying to figure everyone's way out of it.
microsoft/TypeScript#29131.

other people with the same issue:
microsoft/TypeScript#41563
microsoft/TypeScript#11498
microsoft/TypeScript#8516
microsoft/TypeScript#3356
microsoft/TypeScript#4260
DefinitelyTyped/DefinitelyTyped#6091

(Ignore my tone, I'm biased)


edit: According to this TS changelog from 4 days ago, this is not a bug but a "feature". 🤦🏻‍♂️
And they're breaking the internet with it.

@ciaranschutte
Copy link

the "preinstall": "npx npm-force-resolutions" will change the package-lock.json on every npm i but also every npm ci which I don't think we want happening for CI. npm ci shouldn't modify files as far as I'm aware (https://stackoverflow.com/questions/52499617/what-is-the-difference-between-npm-install-and-npm-ci)

Seems that npm-force-resolutions is a way to do yarn style package resolutions. I'm worried this is breaking the npm way of doing things. Maybe we should just fix the issues in the package-lock.json similar to dependabot?

Just my cents, let me know what you're thinking and if I can help @caravinci

@justincorrigible
Copy link
Member Author

justincorrigible commented Nov 24, 2020

Edit: ignore this comment. Modified the PR to disallow npm ci from changing the lock for those resolutions unless instructed to.


You're 100% right, if that's the case! Updating the lock is one of the functionalities expected from npm ci, therefor automation shouldn't be using that to begin with (unless intended for some reason? ¯\(ツ)/¯ ).

Thankfully, we don't suggest using that command anywhere in the documentation, nor have I been able to find it being used in the automation scripts (which as a side note, did help me realise the dockerfile strangely uses yarn, even though this app doesn't have a yarn lockfile.)

As for the resolutions approach, I wasn't aware npm-force-resolutions changes the lockfile every time, apart from the time that gets committed in the repo. If we set the resolutions to a specific fixed version, there's no reason why the versions would change back to vulnerable or incompatible ones; even if the lock does change upon install.
Having a bit of trouble finding references to other people seeing the same issue without npm ci. However, I do remember reading something against changing the lockfiles directly, like most other autogenerated code. This approach tells npm do its job the best way it can.

Rather than using this package, or even changing the lock directly, the ideal scenario here would be as npm-force-resolutions themselves puts it in their documentation:

WARNING before you start
The use case for this is when there is a security vulnerability and you MUST update a nested dependency otherwise your project would be vulnerable. But this should only be used as a last resource, you should first update your top-level dependencies and file an issue for them to update the vulnerable sub-dependencies.

I've seen cases in which updating only the deep inner dependencies has actually broken the top-level ones, because of breaking changes and compatibility. Thoughts?

@justincorrigible
Copy link
Member Author

justincorrigible commented Nov 24, 2020

Update: After locking down the exact resolution versions, not even npm ci is updating the lockfile.

Apart from effective, this approach makes it easier to maintain resolutions later keeping package.json as the single source of truth; and delegating the lock generation to npm, where it belongs.


npm audit shows 0 vulnerabilities using Node v13.14.0

@justincorrigible justincorrigible self-assigned this Jan 4, 2021
@@ -1,4 +1,4 @@
import { GQC } from 'graphql-compose';
import { schemaComposer } from 'graphql-compose';
Copy link
Member Author

Choose a reason for hiding this comment

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

deprecated past graphql-compose@3

@@ -1,16 +1,16 @@
import { TypeComposer } from 'graphql-compose';
import { schemaComposer } from 'graphql-compose';
Copy link
Member Author

Choose a reason for hiding this comment

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

deprecated past graphql-compose@6

Copy link
Contributor

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

Thank you Justin!

@justincorrigible justincorrigible merged commit 1077fd6 into master Jan 5, 2021
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.

Address Persona vulnerabilities
3 participants