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

chore(node): removing our dependency on oas #1054

Merged
merged 13 commits into from
Jul 29, 2024
Merged

chore(node): removing our dependency on oas #1054

merged 13 commits into from
Jul 29, 2024

Conversation

erunion
Copy link
Member

@erunion erunion commented Jul 28, 2024

🧰 Changes

I don't totally understand why we're loading oas in for types on the getGroupIdByOperation method that by all accounts seems to be unused, but because the version of oas we are using is pretty old I'm upgrading it.

Because oas is currently being used solely for getGroupIdByOperation, a method that is not being used anywhere, I am removing getGroupIdByOperation and our oas dependency.

@erunion erunion added enhancement New feature or request node Issues related to our Node SDK labels Jul 28, 2024
@erunion erunion marked this pull request as ready for review July 28, 2024 07:03
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

yeah i just confirmed that getGroupIdByOperation isn't being used anywhere — i say we get rid of it and uninstall oas 🚮

@mjcuva any context on this?

@mjcuva
Copy link
Member

mjcuva commented Jul 29, 2024

I think this was added by @jamestclark! Here's the comments from the initial pr when this was added: https://github.com/readmeio/metrics-sdks/pull/879/files#r1454075743

@kanadgupta
Copy link
Member

ah thanks! re: this comment:

And yeah it's not used directly by the metrics SDK. As the code is very similar to what we already had in the main codebase he idea here is to co-locate all the securityRequirements code here and have the main codebase use this once it's published.

my general take is that because the readmeio package is used by customers, we should only export functions that are useful to end users when possible (as opposed to functions that are exclusively for ReadMe internal use). i also just checked and i can't seem to find any instances of getGroupIdByOperation in our main codebase either. does anybody have qualms about us getting rid of this?

## 🧰 Changes

Noticed a bunch of type errors when running `npx tsc -p test/` from the
`packages/node` directory. Not sure if we should do this typechecking in
CI?

## 🧬 QA & Testing

Do you notice any errors when running `npx tsc -p test/` from the
`packages/node` directory?
@erunion
Copy link
Member Author

erunion commented Jul 29, 2024

kind of agree that we should only keep things that will be used in the sdk within the sdk. i'm going to pull getGroupIdByOperation and oas entirely.

@erunion erunion changed the title chore(node): upgrading oas to the latest release chore(node): removing our dependency on oas Jul 29, 2024
@erunion erunion requested a review from kanadgupta July 29, 2024 16:38
Base automatically changed from feat/native-fetch-support to main July 29, 2024 21:01
@erunion erunion merged commit 2a93cd8 into main Jul 29, 2024
32 of 41 checks passed
@erunion erunion deleted the chore/upgrade-oas branch July 29, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request node Issues related to our Node SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants