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

fix: unified snippet QA #902

Merged
merged 58 commits into from
Sep 6, 2023
Merged

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Aug 17, 2023

🧰 Changes

This PR is dedicated purely to basic cleanup in #879. Any actual changes to the end-user experience (function signature changes, etc.) will come in subsequent PRs. Here are a few callouts:

  • Removed crypto-js in favor of using Node's native crypto
  • Swapped out our dynamic api usage for a code-gen'd, fully typed SDK
  • Tidying up TS types (and it turn fixing some tiny bugs)
  • Adding a couple unit tests

🧬 QA & Testing

Do tests pass and do the changes look good?

kanadgupta and others added 30 commits August 14, 2023 15:59
i had to downgrade to npm@7 for this... absolutely cursed
not sure why VS code didn't flag these the first time 🤔

also i know this is nuts but open to alternatives lol
this is the shit i live for
Co-Authored-By: Aaron Hedges <[email protected]>
@kanadgupta kanadgupta added the refactor Issues about tackling technical debt label Aug 23, 2023
@kanadgupta kanadgupta marked this pull request as ready for review August 23, 2023 17:42
@kanadgupta kanadgupta requested review from mjcuva and erunion August 23, 2023 17:43
@@ -8,10 +8,12 @@
"lib": ["es2019"],
"outDir": "./dist",
"resolveJsonModule": true,
"skipLibCheck": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately this flag is required since the api core doesn't play nicely with TS strict mode (or maybe it's because we support Node 14 in this repo? not sure). i started readmeio/api#696 which could maybe help?

CleanShot 2023-08-23 at 12 35 13@2x

"strict": true
},
"include": [
"src/**/*",
"src/.api/**/*",
Copy link
Member Author

Choose a reason for hiding this comment

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

i was running into build issues with the nested .api/apis/developers/package.json stuff so what i did instead was install api, json-schema-to-ts, etc. in packages/node/package.json and moved the .api directory into the src/ folder so it gets properly included in the TS build

cc: @erunion

Copy link
Member

Choose a reason for hiding this comment

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

wonder if it'd be fine if the codegen'd SDK was JS instead

Copy link
Member Author

Choose a reason for hiding this comment

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

i tried both JS and TS exports and TS even more unhappy with the former

@mjcuva mjcuva merged commit 041d1de into node/unified-snippet Sep 6, 2023
@mjcuva mjcuva deleted the kanad/unified-snippet/ts-strict branch September 6, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants