stripe / stripe-node

Node.js library for the Stripe API.
https://stripe.com
MIT License
3.87k stars 747 forks source link

TypeScript - Property 'plan' does not exist on type 'Subscription' #1794

Closed jonbarrow closed 1 year ago

jonbarrow commented 1 year ago

Describe the bug

The property plan is not accessible on Subscription objects when used in TypeScript. The plan property is, however, present in the Subscription object

Screenshot from 2023-05-18 16-16-57

Screenshot from 2023-05-18 16-16-33

To Reproduce

  1. Create an instance of Stripe
  2. Request a Subscription object
  3. Try to access the plan property

Expected behavior

For the plan property to be accessible

Code snippets

No response

OS

Ubuntu 22.04

Node version

Node v18.12.1

Library version

stripe-node v12.5.0

API version

2022-11-15

Additional context

The documentation for the Subscription object also does not list the plan property, and several others https://stripe.com/docs/api/subscriptions/object

remi-stripe commented 1 year ago

@jonbarrow The plan property on Subscription has been deprecated for many years now so it's expected that it's not in types and undocumented. See https://github.com/stripe/stripe-node/issues/1517#issuecomment-1221355535 for a longer explanation as to why. We recommend using items.data[xxx].price instead!

jonbarrow commented 1 year ago

@jonbarrow The plan property on Subscription has been deprecated for many years now so it's expected that it's not in types and undocumented. See https://github.com/stripe/stripe-node/issues/1517#issuecomment-1221355535 for a longer explanation as to why.

We recommend using items.data[xxx].price instead!

Regardless on its deprecation, the data is still present and should honestly be documented as such. This property is still being included in data made even today, and it should still be accessible until it actually is removed. The documentation should also reflect that it exists but is deprecated. It's a bit nonsensical to have data which clearly still exists but is being arbitrarily withheld from developers. Improper documentation as to what the objects actually return will just lead to more issues like this

jonbarrow commented 1 year ago

@remi-stripe I definitely understand deprecating parts of an API, but without proper documentation of deprecated features developers can run into footguns. The data is still available in the API, and so without proper documentation it's unclear as to what the data is or what the support plan is or anything like that. So long as the data is still returned by the current version of the API it should at the very least be documented as a deprecated feature

Additionally, since the property is still returned it means that anyone not using TypeScript and is instead using plain JavaScript (or any other non-official library such as a custom client, cURL, etc) could very easily see the property and use it, without knowledge of it's deprecation or what it's level of support actually is. This was the very situation I was in, moving plain JavaScript code to TypeScript only to find the property is not typed

To make matters worse, this property was never actually marked as deprecated. It was just removed from documentation. You can see here on August 3rd, 2020 the property is still documented under More attributes, with no warning of deprecation or what to use instead. Then a week later on August 10th, 2020 is outright removed from documentation

I am now operating under the assumption that more objects have these random deprecated properties which were just removed from documentation one day. I would suggest at least documenting these properties and marking them as heavily deprecated, directing users to the new replacement. Even if they remain unaccessible in these libraries, having the information as to what the data is would surly solve problems like these

remi-stripe commented 1 year ago

@jonbarrow Thanks for the thoughtful feedback! I definitely hear you and it's something we do want to get better at in the future.

What's important to note is that we won't just remove this from the API without warning. What we would do is in the future release a new API version that would stop returning that property while keeping it on old integrations. So it wouldn't break any code or impact existing integrations and would only apply to newer integrations

I do think having it marked as deprecated in the types can be more harmful for developers in general though. Over the years, we have deprecated many properties like that that we've undocumented. This makes it easier for existing integrations to keep evolving and adopting new API versions for other important breaking changes that we released without having to change those parts. But it makes it so that newer integrations often ignore those undocumented properties and focus on the greener/better documented paths we have.

Definitely not perfect though and something we've regularly tried to improve across all our libraries!

jonbarrow commented 1 year ago

@remi-stripe I am unsure what you mean by this:

we won't just remove this from the API without warning. What we would do is in the future release a new API version that would stop returning that property while keeping it on old integrations. So it wouldn't break any code or impact existing integrations and would only apply to newer integrations

I provided an example where a feature was removed without warning from the documentation? Additionally, my app was made significantly after both the deprecation and the documentation changes (the deprecation happened in 2017, the documentation change in 2020, and I started using Stripe in 2021), yet all the data and responses from the API I receive still have this property. So it does not seem as though this was removed from future integrations as you say? I have been using this undocumented, deprecated, property for close to 2 years now with no issues thus far?

I do think having it marked as deprecated in the types can be more harmful for developers in general though

I understand your position, but respectfully disagree. If not documented in the types, I strongly feel like it should at least be documented in the documentation on the Stripe website. Not having any documentation at all for some properties is what will, in my opinion, harm developers more and create footguns. This is especially true in cases like when using this library with vanilla JavaScript, which does not have the type defs provided by TypeScript to not allow access to these properties, or when using tools like cURL (of which there are examples for using in the documentation, and developers will) which will show them as well

As per my last message I believe a better option to avoid all confusion is more complete documentation, not less documentation. It would be much more clear what the data is if it was documented with a proper warning, making it clear that the feature exists, yes, but is subject to removal or change and is deprecated, as well as guiding the user to the new API. Additionally if the feature should be removed, an actual road map for the features removal should be in order

Right now the reality of what happened is that the property was documented fully at one point, with no clear indication that it was deprecated, and then suddenly removed a week later, less than 3 years ago from the documentation. I would still consider apps made within the past 2-3 years to be relatively modern apps, and many of which may have used that documentation when it existed under the assumption that it was still in long term support

Without proper notice or warnings of deprecated features it harms developers attempting to either do exactly what I did (move from JavaScript to TypeScript), those who are trying to update older codebases (as the data they receive is still valid in their old apps, but there's no clear indication as to why it was removed from the documentation and no path to follow for the new API), and those who use the API documentation to make custom clients for example in environments where an official SDK cannot run or in a language which is not currently supported

Seeing as how the deprecation process has gone with this property, it brings into question whether or not I reliably use any feature provided by Stripe, as now it's clear that at any moment it can simply be removed from the clients/documentation even if the data itself is technically still accessible

remi-stripe commented 1 year ago

I provided an example where a feature was removed without warning from the documentation?

It's removed from the docs, it's still here in the API and works. We won't change that, since it'd break 10 years of integrations. At a future date, we will remove plan and quantity as a property from the Subscription resource but this will be done in a new API version. All existing integrations using an older API version would still see those properties. An example of a similar change can be seen on 2020-08-27 where we removed tax_percent from most resources in favour of tax_rates (as the TaxRate API had been live for multiple years). Old code on old API versions can still see and use tax_percent without any change.

many of which may have used that documentation when it existed under the assumption that it was still in long term support

I think that's the crux of the misunderstanding. We are not removing this, and we won't email you in a week or a year asking you to stop relying on it. That's not how we usually do breaking changes and instead we put them behind new API versions (like the example I gave above).

Seeing as how the deprecation process has gone with this property, it brings into question whether or not I reliably use any feature provided by Stripe, as now it's clear that at any moment it can simply be removed from the clients/documentation even if the data itself is technically still accessible

In this specific case the data is removed from the Typescript definitions which wouldn't/shouldn't break your code. The data is still in the API and accessible in stripe-node, or stripe-python for example.

I don't disagree we could handle deprecations better in the future. We have flagged things as deprecated before and then removed them a major version or two later. Here the example you took is quite old (deprecated in 2017, removed in 2020). That clean up was partly related to the introduction of Typescript definitions in the first place which shipped in January 2020 here. Part of this effort allowed us to then identify a lot of old features that were discouraged and remove them from types to limit the risk of adoption which has worked overall based on how many new integrations rely on newer features like passing items instead of a top-level (and deprecated) plan parameter. An example can be seen here where we flagged that shipping_rates option as deprecated and will remove it from the types (not the API) in a future version.

We do hear your feedback and we do agree that we need to get better at this and we're working on it!

jonbarrow commented 1 year ago

@remi-stripe I would like to start this off by thanking you for the calm and understanding replies you have given to this issue thus far. Despite the fact that we clearly have fundamental disagreements on what the right path for this is (it is clear that the Stripe teams stance is to leave some deprecated features undocumented, of which I firmly disagree with), I appreciate being given the ability to have a respectful dialogue about it and I respect your willingness to accept feedback and potentially make changes, and I hope that my replies meet you with the same level of respectfulness. At the end of the day my goal is to help improve the developer experience for everyone and to help Stripe improve moving forward

It's removed from the docs, it's still here in the API and works

I appreciate the clarification on this bit. I had wrongly assumed you meant removed from the documentation, hence my reply. Though on that note, my big issue I have been having is with the documentation, not so much the API itself. Is there a reason why things are not being marked as deprecated in the documentation itself? That is one one of the biggest issues I have, if not the crux of all this. Referring back to the example I showed earlier, regardless of why it was removed from the documentation and ignoring the TypeScript types (later on you mention the plan property being removed due to the introduction of the TypeScript types, however this is irrelevant to the documentation itself being lacking for years prior), for years the top-level plan property was deprecated and developers should be using newer APIs, however this was not clear whatsoever based on documentation alone. This information could only be found out via additional research after the features removal, which may or may not have even given a developer the information they were looking for. The property was fully documented as if it was still in a stage of long time support, and then within a week was removed from the documentation. There was no notice in the documentation itself showing that the feature was deprecated at all, let alone any mention of what to use instead

I find this lack of complete documentation to be very misleading to developers. How should a developer making a new app in the weeks leading up to that features removal from the documentation know that that feature should no longer be used in favor of new APIs, if it is not clearly marked as deprecated with some kind of note guiding them to where they should look instead? Building off of this, the fact that it was removed from documentation without warning but continues to function could lead to a group of confused developers making their apps around that time, who would likely need to reefer to the documentation multiple times only to find that one day properties are there, and another they are not, for seemingly no reason. Yet the data is still present in their responses. They may then ask themselves, "Do we keep using this property?", "Why was this removed?", "Is this still reliable? What do we use instead if not?". These questions remain unanswered and leave a group of developers in the dark due to lacking documentation

I am no longer speaking specifically about the plan property on Subscription objects mind you. I was bringing up example scenarios for what could have happened then, but because this can always happen again in the future with other properties, and I would like to try and help mitigate those issues from happening in the future

We are not removing this, and we won't email you in a week or a year asking you to stop relying on it.

I am aware. There seems to be a misunderstanding as to where the goalpost lies here, and I apologize if I have made that unclear. I am not worried about the features removal from API responses, my replies have generally been geared towards their removal from documentation and the quality of documentation overall (not just limited to this one property). When I said many of which may have used that documentation when it existed under the assumption that it was still in long term support, I meant long term support to mean "the current recommended API", meaning that there was no indication in the documentation at that time that the top-level plan property was no longer the recommended way of accessing that data, and that the newer APIs are the suggested way

In this specific case the data is removed from the Typescript definitions which wouldn't/shouldn't break your code. The data is still in the API and accessible in stripe-node, or stripe-python for example.

I do not understand what you mean here. This is false, and was actually the reason why this issue was opened originally. Yes the data is still in the API responses, but the fact that it is removed from the TypeScript type defs does break code. TypeScript throws an error that the property being accessed does not exist on the type, and will not compile with this error. It breaks code by being removed. I left screenshots of the error that TypeScript throws when trying to access the missing property in my opening post on this issue

remi-stripe commented 1 year ago

Thanks for the positive feedback on my replies! Text communication is not always easy so I'm glad you got the intent I wanted to put into my words. We genuinely want to improve this! It's something the engineering team who maintains the client libraries/SDKs like stripe-node think about a lot. Unfortunately, there are things we can realistically achieve and ship while others are more a long term goal we want to get better at and this is one of them. For now, while definitely not perfect, undocumented old properties/parameters has helped lower the number of integrations that rely on those quite drastically even if some still look at the raw JSON/payload and notice them.

Is there a reason why things are not being marked as deprecated in the documentation itself? That is one one of the biggest issues I have, if not the crux of all this.

It's really mostly a cost basis. There are likely hundreds of properties and parameters across our API that we consider deprecated, but where removing them via an API version would be too costly (both for us and for developers). For now, we have settled on undocumenting those which drastically lowers their overall usage and adoption. The alternative would be that new developers looking at the Subscription API and using the Price API (which shipped in 2018) see a top-level plan and quantity marked as clearly deprecated for years and likely get more confused overall. And in numbers, we'd likely have 100 developers confused about deprecated properties for 1 developer using that property from their old code or some old guide.

I find this lack of complete documentation to be very misleading to developers. How should a developer making a new app in the weeks leading up to that features removal from the documentation know that that feature should no longer be used in favor of new APIs, if it is not clearly marked as deprecated with some kind of note guiding them to where they should look instead?

If you use types, you don't see plan so you wouldn't use it. If you use stripe-java, stripe-dotnet or stripe-go, they are strongly typed and you can't see/access plan as a property. If you follow our docs while you integrate, we talk to you about the Price API, the price parameter/property across those APIs and you shouldn't really look at something named plan. You might which often happens to developers who basically dump the whole object to see what it looks like and debug it and many of those would see plan, not see it in the Docs and look for an equivalent which is items[data][0][price] and use that since they pass the Price id price_123 in the price parameter in the first place.

Now some developers would incorrectly see plan top-level, rely on it and use it. In a way, it's totally fine because their code will continue to work and we won't change this. We might, a week later or 2 years later ship a new API version that removes it. But it'd only break their code if they actively updated the code to use the newer API version. In that case they would read our API changelog and it'd be clearly called out that the top-level plan property on Subscription was removed and to use items[data][0/1/...][price] instead.

Let's say yesterday we had just released an API version 2023-05-18 that did exactly that. The property would not exist anymore on that version so it wouldn't be anywhere in our types either since those map to the latest API version. A new developer might be on an old API version and still see plan top-level and not see it in the types or the docs and they'd be plausibly as confused. Having marked the property as deprecated for years before wouldn't help those developers either because when they start integrating today the types would not have plan as deprecated as it's not in the API anymore for the latest API version.

to mean "the current recommended API", meaning that there was no indication in the documentation at that time that the top-level plan property was no longer the recommended way of accessing that data, and that the newer APIs are the suggested way

Our main docs about Subscriptions have used the Price API for many years now, we don't really talk about the Plan API anymore. It's here for compatibility, and deprecating it fully is really costly and something we've deferred for now, but I would say the majority of the developers who build new integrations today only use the price properties and parameters across our API.

I do not understand what you mean here. This is false, and was actually the reason why this issue was opened originally. Yes the data is still in the API responses, but the fact that it is removed from the TypeScript type defs does break code. TypeScript throws an error that the property being accessed does not exist on the type, and will not compile with this error. It breaks code by being removed. I left screenshots of the error that TypeScript throws when trying to access the missing property in my opening post on this issue

But that happens while you're upgrading your library or your code to switch to those types right? That doesn't happen in production after you deploy a change. That's the point I was trying to make. As a developer, you either upgraded stripe-node and its types to a newer version or you switched to using types and that code has a compile error you needed to investigate.

jonbarrow commented 1 year ago

@remi-stripe (do I need to keep pinging you? I apologize if this is unnecessary) I appreciate the level of detail you went into with the explanations, however due to the amount of information there and how much of it is related to each other I hope you'll forgive me in not directly quoting specific parts (otherwise I would wind up quoting whole paragraphs, and I don't think either of us would want to read or write that long of a message :P )

I definitely understand where you are coming from, but it feels like there may still be a slight misunderstanding/miscommunication going on. Yes this issue was originally made on the topic of the Plan API and top-level plan on Subscription objects, but I believe the discussion has outgrown that specific context. Most of my previous message was supposed to apply very broadly, both in time and API coverage. Meaning my concerns are not limited to one specific API/property nor at one specific period of time

I am aware that new developers are less likely to use a feature which has been undocumented for years now, but that was not my point of concern. My concern is that any property Stripe offers could at any time, now or in the future, be removed at any point (and the fact that you mentioned hundreds of other properties and parameters you consider deprecated in this way, is exactly why I am concerned about this)

I used the Plan API here mostly as an example since it's the issue I specifically ran into, but my intent was to use it as an example to illustrate what I believe to be a much broader issue. That issue being a lack of a proper roadmap for developers with regards to deprecated features

As I said previously here, I am aware that new developers are less likely to use deprecated features which have been undocumented for years. My point is that, until the feature has been undocumented for years this feels like a bit of a disingenuous point to try and make. I was putting myself in the shoes of a developer making their app in August of 2020. If I were a developer in that time, I very well could have built an app which relied on the Plan API, after seeing the top-level plan property, believing it to be reliable since the documentation does not explicitly say the feature is deprecated. Even with the Price API having already rolled out, there is no mention of moving away from the Plan API in the documentation itself (that I could find). If I were a developer at this time, I could have very well assumed that both are perfectly valid and supported APIs, since no one said otherwise and I trusted the documentation, only to then find out a few weeks or even days later that the property is missing

In my opinion this leads to a lot of unnecessary confusion and back tracking for the developer, both in terms of finding new documentation and wasting time rewriting code, since if a proper warning was given beforehand then as the developer I would have very explicitly known to not use that API, saving me development time

Again to be clear, I am only using the Plan API as an example to illustrate what I believe to be a much broader issue. As a hypothetical, lets use the Coupons API. Say that API is deprecated in favor of a new, made up, API called "Deals" instead. Stripe plans to remove the Coupons API in 3 months in favor of the "Deals" API which has already rolled out, but references to the Coupon API are still found in other parts of the documentation, and I begin working on my app right now which heavily relies on the existing Coupon API. As it stands right now, there would be no mention of a deprecation of this API, no warning that it will be undocumented, and no guidance within the deprecated API itself as to where to look to for modern integrations (the "Deals" API). 3 months later, after spending all that time working on my app, I need to check something related to coupons only to find that the API documentation is completely missing. I now have to do additional work to find out what happened to it, and then spend however long undoing 3 months of work when I could have just used the "Deals" API to begin with, if I was properly directed to it

That hypothetical is the crux of the issue I have with the state of documentation. It's not clear based on the documentation alone what actually should and shouldn't be used, and undocumenting chunks of the API only helps developers farther in the future while alienating and causing issues for groups of developers in the moment, which could have been greatly mitigated if they were directed away from the deprecated features to begin with when they saw them. In my hypothetical, marking documentation referring to the Coupons API in a clear way, such as with a vibrant red box or outline and a message clearly stating "This API is deprecated and will become undocumented soon. Please see the Deals API for modern integrations" would have solved the issue

But that happens while you're upgrading your library or your code to switch to those types right? That doesn't happen in production after you deploy a change. That's the point I was trying to make. As a developer, you either upgraded stripe-node and its types to a newer version or you switched to using types and that code has a compile error you needed to investigate.

Yes this is true, I am moving to using types now. I was commenting on specifically where you had said removed from the Typescript definitions which wouldn't/shouldn't break your code, which is incorrect. Removing types from existing code does break that code. Though I feel like that may have just been poor wording or a miscommunication, which is why I asked for the clarification

remi-stripe commented 1 year ago

Yeah sorry, I focus on Plan specifically because we need a concrete example to talk about it. Most of what I said applies to many parts of our API over the years.

Your Coupon vs Deal example is quite good so I'll run with it. When we plan the Deal API internally, this might take weeks (or months) to design this right. We'll build it, run a small beta with users, do UXRs to figure out if the new design/naming resonates. We'll have reviews internally with other engineers deeply familiar with our API to decide how we can roll this out safely without causing disruption or confusion. At that point we might think "oh the Coupon API will be deprecated at some point" but it's not something we can tell developers since the Deal API is not even public. Once that API goes public, we also can't tag the Coupon API as deprecated either. There are 10 years of integrations building on top of that API, it will not go away at all. If we marked it as deprecated, this would cause a lot of confusion in many ways for people especially as the Deal API is so new and might be limited at first too. It might take months (or years) to get to feature parity and for the Deal API to suddenly cover everything the Coupon API did and then do even more and better where it's worth the migration for many developers. In the meantime, we have to deal with both APIs existing while trying to nudge new integrations to the better path forward.

In a way, that example has happened multiple times before. The Plan API was replaced by the Price API back in 2020. All our guides use the Price API, the new products like Checkout or Quotes use Price everywhere. But in our API and our docs both Plan and Price APIs are fully documented as existing APIs. Because when we released the Price in 2020, all integrations only knew about Plan as a concept. Just the existence of Price as an API caused confusion and churn for existing platforms and plugin developers who were wondering what to do, why things changed, etc. Almost 3 years later, both APIs are still here because that is crucial to our API design that we support both. We do try to document this well, but we can't realistically say "the whole Plan API is deprecated" because this wouldn't be well understood or received. So we live in a middle ground where both exists, we promote the new ones, 95% of all new integrations use the new one and don't care about the old one, but all integrations built in the past, and all new users that rely on those older integrations still work flawlessly. If you're a new user and you use a Connect platform that manages your Subscriptions, they might use the Plan API and you will have full access to it even though you started on Stripe 3 years later.

Without getting too deep into the details here, we have deprecated entire APIs over the years where we removed all docs, removed them from client libraries/SDKs but we still support those for old integrations. And sometimes just the mention of calling it deprecated worries developers more than just not documenting it. There's unfortunately a wide range of reactions/preferences/habits that go into it. And some of it is also deeply related to your own programming language/ecosystem in general and how you approach breaking changes.

This is getting a little bit meta at this point though and I hope you can tell how passionate I am about this and it's something we talk about constantly internally :)

Ultimately, I hear your feedback and I agree with most of it. In a perfect world, we'd be able to fully document all of those historical quirks, ensure new developers don't use them by mistake, while supporting older integrations to see why they use certain parts. But we also have to contend with the size of our API, its evolution over many years and how much work can realistically go into each part from building and maintaining those APIs to docs, to public samples, to client libraries/SDKs, to StackOverflow answers from years before, etc. We do mark things as deprecated regularly, we do remove things in API versions, but overall there is real inherent upside/value into the "undocument but keep it supported" approach we've taken. We've honed this over the years and we're quite particular about when we use which tools based on a variety of reasons from internal metrics, to adoption of the feature, to upside of the new approach. And in this case, undocumenting has been the best path forward for us though we definitely see it causes confusion like the one you're describing and it's not perfect.

jonbarrow commented 1 year ago

It appears as though we have reached our conclusion, then. You are right that I absolutely do see how passionate you are about this, I never doubted that nor did I doubt that this is something heavily discussed internally :) I was not trying to presume that these decisions were made rashly or without thought, and I apologize if that seemed to be the case

You make many valid points and I do not aim to discount those. I appreciate you taking the time to fully discuss this with me, as I now have a better understanding into the decision making process for these things, and I hope my feedback does help build a better product for future developers in some form or another. Unfortunately without access to the metrics you all have I doubt I will ever be able to truly understand everything, and will just leave it up to faith that the overall correct decision is being made for your product, even if I would personally do things differently. Though I can't ignore the good point that you made about the scale of the API and how wide it's reach is. So who knows maybe, I wouldn't do things differently if my tools were on the same scale and I had the same metrics data you have

Regardless I feel as though this has been a very worthwhile discussion to have and I hope you have taken just as much back from this as I have. Thank you for the opportunity to hear me out and for listening to my feedback

remi-stripe commented 1 year ago

I really enjoyed our discussion. The whole "should we just undocument this for now" is something we've discussed many times and it's something we don't all agree on. Usually it starts with "should we undocument it and then do an API version in a quarter to remove it?" and then sometimes the quarter becomes 3 years and here we are 😓

I'm going to make sure a lot of people read through this conversation internally too and it gives us a lot to mull over and it makes the theoretical "what if developers are confused by this?" a lot more realistic. Please keep the feedback coming :)

jonbarrow commented 1 year ago

I'm glad to hear it! Always happy to help and be apart of the process, I'll be sure to drop any other feedback I may have in the future!

markwoodward23 commented 1 year ago

Been really interested following this debate after raising the same issue just a few days ago! I only have one other thing to add. Out of the approaches you mention @remi-stripe - I do see the benefits of undocumenting but keeping it supported, and you could argue that if the way you handle deprecation is to release new major versions anyway you could argue leaving it marked as deprecated but documented vs undocumenting doesn't really make a difference. I do think there's a lot of people like @jonbarrow and I who do read the docs, but who also use tutorials (3rd party and stripe) or just Intellisense to explore APIs. Call us crazy for not reading the docs but I've found I only needed to dip into the docs for formatting tips occasionally and that's testament to the good design you clearly put in. Though - when you log out the response and see the field you want but TS throws a wobbler you end up here :)