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.

Sign up for GitHub

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

Jump to bottom

refactor: avoid semver where possible #1309

Merged
merged 7 commits into from
Feb 3, 2021
Merged

refactor: avoid semver where possible #1309

merged 7 commits into from
Feb 3, 2021

Conversation

TrySound
Copy link
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 as code owners 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
JustinBeckwith
JustinBeckwith approved these changes Sep 19, 2020
View reviewed changes
Copy link
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
Contributor Author

TrySound commented Sep 19, 2020 •
edited
Loading

Yes, I found isSupported method which is used for plugins.
https://github.com/googleapis/cloud-trace-nodejs/blame/master/doc/plugin-guide.md#L10
https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/trace-plugin-loader.ts#L130

@bcoe
Copy link
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
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
Contributor Author

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

@TrySound
refactor: avoid semver where possible
4660910 
Checking node version is not necessary anymore with "engines" field in
package.json which prevents from installing this package with legacy node.
@TrySound
Throw when require in unsupported node version
8a82f78
@bcoe
Copy link
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
Contributor Author 

$ npm run fix

> @google-cloud/trace-agent@5.1.1 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
Contributor Author

Hm... fix does not have any effect

@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 3, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 3, 2021
@bcoe bcoe merged commit 4c05cae into googleapis:master Feb 3, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 4, 2021
@release-please
chore: release 5.1.2 (#1330)
cbdbb6c 
🤖 I have created a release \*beep\* \*boop\* 
---
### [5.1.2](https://www.github.com/googleapis/cloud-trace-nodejs/compare/v5.1.1...v5.1.2) (2021-02-04)


### Performance Improvements

* **deps:** avoid semver where possible ([#1309](https://www.github.com/googleapis/cloud-trace-nodejs/issues/1309)) ([4c05cae](https://www.github.com/googleapis/cloud-trace-nodejs/commit/4c05caeb3910657b3eb413da61aac03cb321bd7b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@release-please release-please bot mentioned this pull request Jun 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TrySound @bcoe @JustinBeckwith @yoshi-kokoro