docs: migrate guides to TSDoc references #6100
Conversation
🦋 Changeset detectedLatest commit: 5dc2813 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| constructor() { | ||
| super({}) | ||
| constructor(_) { | ||
| super(_) |
There was a problem hiding this comment.
@adrien2p can you confirm if it's ok to make this change? I needed to add the constructor to the AbstractTaxService for documentation purposes but the original constructor implementation caused a TypeScript error related to {} not matching type MedusaContainer.
There was a problem hiding this comment.
what I usually do is the following
class SystemTaxService extends AbstractTaxService {
static identifier = "system"
constructor(...args: any[]) {
super(...arguments)
}Why? Because here what we are saying is that the SystemTaxService has a defined public constructor that can take any arguments and we pass all the given arguments to super (the base class). Now, maybe we would like to define specifically what arguments this class can take if someone wanted to extend it or just to specify what it is expected.
There was a problem hiding this comment.
I think it should be this right?
constructor(...args: any[]) {
super(args)
}args instead of arguments.
I still get an error for incompatible types. For reference, this is how I've written the constructor:
medusa/packages/medusa/src/interfaces/tax-service.ts
Lines 237 to 242 in f019d4e
I based this implementation of both the constructor in the abstract class and in the system tax service on the implementation for the system payment provider.
There was a problem hiding this comment.
you need to spread the args. arguments is a special js keywords that hold all the arguments passed to a function. args here are the same as arguments. The difference is that if you always want to pass all the args provided by to the constructor arguments will always works.
In your changes above you are passing an array to the parent constructor
There was a problem hiding this comment.
I know, but I get this TS error when I use your codeblock:
A spread argument must either have a tuple type or be passed to a rest parameter.ts(2556)
There was a problem hiding this comment.
you can add ts-ignore. If you look in the code base we are already doing that
There was a problem hiding this comment.
Got it, I've added that. Thanks! 🙏
|
@adrien2p can I get your approval on this one? |
|
Holy smokes, this diff is something. But let's get it in 😄 |
|
@olivermrbl most of the big file changes are in the generated references in the docs, which you don't need to review. You can just review files under the |
| * @example | ||
| * // ... | ||
| * import { | ||
| * AbstractPriceSelectionStrategy, | ||
| * CustomerService, | ||
| * } from "@medusajs/medusa" | ||
| * type InjectedDependencies = { | ||
| * customerService: CustomerService | ||
| * } | ||
| * | ||
| * class MyStrategy extends | ||
| * AbstractPriceSelectionStrategy { | ||
| * | ||
| * protected customerService_: CustomerService | ||
| * | ||
| * constructor(container: InjectedDependencies) { | ||
| * super(container) | ||
| * this.customerService_ = container.customerService | ||
| * } | ||
| * | ||
| * // ... | ||
| * } | ||
| * | ||
| * export default MyStrategy | ||
| */ | ||
| protected constructor( | ||
| protected readonly container: MedusaContainer, | ||
| protected readonly config?: Record<string, unknown> // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| ) { | ||
| super(container, config) | ||
| } | ||
|
|
There was a problem hiding this comment.
the introduction of the constructor (before this PR it was auto extended from TransactionBaseService which had container: any) broke the example written here above (and the one from the documentation). I don't know now how to inject dependencies as passing InjectedDependencies Object is not allowed by the type MedusaContainer now anymore.
Related issue: https://discord.com/channels/876835651130097704/1201534153896763472/1201534153896763472
There was a problem hiding this comment.
Thank you for pointing this out @tobiaskraus , I'll open another PR to fix it. In the meantime, you can instead use super(arguments[0]) instead of super(container)
Edit: Turning this into a PR holding all left migration of guides to references.
Important
This PR includes changes to some plugins and abstract classes constructors. Check the changesets for more details.
This PR includes changes from the PRs #6121 , #6116 , #6115 + the following changes:
Tax Provider
Notification Provider
File Service
Search Service
Medusa Configurations