Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

refactor: avoid semver where possible#1309

Merged
bcoe merged 7 commits intogoogleapis:masterfrom
TrySound:avoid-semver
Feb 3, 2021
Merged

refactor: avoid semver where possible#1309
bcoe merged 7 commits intogoogleapis:masterfrom
TrySound:avoid-semver

Conversation

@TrySound
Copy link
Copy Markdown
Contributor

Checking node version is not necessary anymore with "engines" field in
package.json which prevents from installing this package with legacy node.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@TrySound TrySound requested review from a team September 18, 2020 22:30
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 18, 2020
Copy link
Copy Markdown
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'm curious - did you see other places we were using the module? Wondering if we can remove it from package.json all together.

@product-auto-label product-auto-label Bot added the api: cloudtrace Issues related to the googleapis/cloud-trace-nodejs API. label Sep 19, 2020
@TrySound
Copy link
Copy Markdown
Contributor Author

TrySound commented Sep 19, 2020

@bcoe
Copy link
Copy Markdown
Contributor

bcoe commented Sep 21, 2020

👋 most folks do not have --engines-strict enabled for their npm i, so engines is actually not a great way to protect users from these types of issues. Throwing a clear error message is a much better user experience.

I'd be open to dropping the SemVer dependency, but I would want to see it instead replaced with logic that checks the major version, like this.

@bcoe
Copy link
Copy Markdown
Contributor

bcoe commented Nov 23, 2020

@TrySound this has stalled for a little while, did you have any interested in updating the PR to still throw on unsupported engine versions?

@TrySound
Copy link
Copy Markdown
Contributor Author

Oops, sorry. Forgot about it. Will update today.

Checking node version is not necessary anymore with "engines" field in
package.json which prevents from installing this package with legacy node.
@bcoe
Copy link
Copy Markdown
Contributor

bcoe commented Nov 23, 2020

@TrySound looks reasonable to me, could I just bother you to run npm run fix, for the benefit of the linter.

@TrySound
Copy link
Copy Markdown
Contributor Author

$ npm run fix

> @google-cloud/[email protected] fix /.../cloud-trace-nodejs
> gts fix

version: 14

/.../cloud-trace-nodejs/samples/app.js
  20:11  error  "@google-cloud/trace-agent" is not found  node/no-missing-require

/.../cloud-trace-nodejs/samples/snippets.js
  18:9  error  "@google-cloud/trace-agent" is not found  node/no-missing-require

✖ 2 problems (2 errors, 0 warnings)

@TrySound
Copy link
Copy Markdown
Contributor Author

Hm... fix does not have any effect

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: cloudtrace Issues related to the googleapis/cloud-trace-nodejs API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants