-
Notifications
You must be signed in to change notification settings - Fork 25
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
refactor(core): moving the core library into an isolated package #725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these SDK fixtures had changes because I slightly bumped our oas
dependency and we've fixed a number of issues since the release that this was using:
- Support for
null
mixed types - Addition of a
Default: <default value>
suffix to enum descriptions. - Improved unused schema removal.
import Oas from 'oas'; | ||
import APICore from 'api/dist/core'; | ||
import definition from '../../../__fixtures__/definitions/operationid-quirks.json'; | ||
import APICore from '@api/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving core also lets us do this clean require instead of having to set this up in exports
.
"@types/lodash.setwith": "^4.3.7", | ||
"@types/lodash.startcase": "^4.4.7", | ||
"@types/prettier": "^3.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove this when I removed Prettier support last month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on bringing in knip? Happy to PR that later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully into bringing in Knip. Will ticket something on the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figure this readme doesn't need any usage or installation info since you won't ever use this package outside the scope of a codegen'd SDK.
{ | ||
"name": "@api/core", | ||
"version": "1.0.0", | ||
"description": "The magic behind `api` 🧙", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could stand to give this a better description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I dig it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really excited for these changes, looking good for the most part! But there are a lot of random fixture changes in the api
subpackage and I can't seem to pinpoint why 🧐
{ | ||
"name": "@api/core", | ||
"version": "1.0.0", | ||
"description": "The magic behind `api` 🧙", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I dig it!
@@ -0,0 +1 @@ | |||
import '@api/test-utils/vitest.matchers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this
Co-authored-by: Kanad Gupta <8854718+kanadgupta@users.noreply.github.com>
🧰 Changes
Installing a codegen'd SDK right now installs all of
api
which means that our users are getting all of our dependencies for codegen installed (ts-morph
,commander
,@readme/openapi-parser
, the works). Since that's not ideal, and definitely not necessary for an SDK that's been generated, I'm splitting our core library out into a new@api/core
package that SDKs will then install.closes RM-7992