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

Vendor "graphql-client" and bump "is-my-json-valid" to 2.20.6 #3497

Closed
wants to merge 3 commits into from

Conversation

jesec
Copy link

@jesec jesec commented Nov 10, 2021

This PR contains:

A BUGFIX

Describe the problem you have without this PR

#3495

npm emits a bunch of security warnings.

Notes

I also migrated "graphql-client" to TypeScript. Though, there is still one warning left due to "pouchdb-fetch" (pouchdb/pouchdb#8408).

package.json Outdated
"is-electron": "2.2.0",
"is-my-json-valid": "2.20.5",
"is-my-json-valid": "^2.20.6",
"isomorphic-fetch": "^3.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not use version ranges.

Copy link
Author

Choose a reason for hiding this comment

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

It is not a good idea for a library to pin dependencies. It prevents dependents from bumping packages, should the need arise, or if there is a security issue. In other cases, it might lead to out of sync dependency trees. Last, if you are busy for a period of time and can't publish new versions, the pinned dependencies will remain a PITA for all dependents.

Anyways, I will leave that decision to you. I pushed the commit you requested.

Copy link
Owner

@pubkey pubkey Nov 14, 2021

Choose a reason for hiding this comment

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

Yes all this is true in theory. But in practice, stuff breaks all the time, it did so in the past and it will so in the future.
When a new version is released, the renovate bot will make a PR to this repo with the updated version.

I know that this is not the common opinion, but I had this discussion in the past so often.
When you add a version range and someone runs npm install rxdb, you cannot predict which subdependency version will be installed. And then they come and create issues because nothing works for them. Also a package-lock would not help because the package-lock is not part of the npm package.

@pubkey
Copy link
Owner

pubkey commented Nov 10, 2021

I am not sure if this is a correct use of the license, when we just copy the whole code over to the RxDB repo.
RxDB uses apache2, while graphql-client has set ISC on the package.json, but the repo has no license file.

@jesec
Copy link
Author

jesec commented Nov 10, 2021

I am not sure if this is a correct use of the license, when we just copy the whole code over to the RxDB repo. RxDB uses apache2, while graphql-client has set ISC on the package.json, but the repo has no license file.

That project made it clear that it is under ISC with the license field in package.json.

Otherwise, if a repo indicates no license, that means no license. You can't use it in any fashion (not even as a dependency) when that's the case.


if you have concern about license compatibility, ISC is permissive, and is compatible with Apache 2.0.

@pubkey
Copy link
Owner

pubkey commented Nov 11, 2021

I am really against copying this code into the RxDB repo.
ISC might be compatible, but it adds more complexity for users when they want to evaluate if the license is ok for them.
Also I would have to maintain this code for a long time which I do not want.
Isn't there another library that we can use?

@jesec
Copy link
Author

jesec commented Nov 12, 2021

I am really against copying this code into the RxDB repo.
ISC might be compatible, but it adds more complexity for users when they want to evaluate if the license is ok for them.
Also I would have to maintain this code for a long time which I do not want.
Isn't there another library that we can use?

There might be. But that’s not going to be a drop in replacement.

It is really not that complicated. It is the same thing when you choose to depend on the package. ISC is in no way viral.

@pubkey
Copy link
Owner

pubkey commented Nov 14, 2021

I really do not want to copy over code. I did this in the past. Years on from now, you will long be gone but I will still be sitting there maintaining the code.
Also most non-side-project projects I work with are full of people who are afraid of using any open source tool. I will not make this more complicated by mixing up licenses.

Isn't there some graphql request library that other projects already rely on?

@pubkey pubkey closed this Nov 24, 2021
@pubkey
Copy link
Owner

pubkey commented Nov 24, 2021

Closed since no answer.
is-my-json-valid was updated in ded4fdb

@pubkey
Copy link
Owner

pubkey commented Nov 1, 2022

Hi @jesec
I have to admit, you where right, I was wrong.
The graphql-client will be replaced: #4101

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

2 participants