iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
616 stars 210 forks source link

start using `@since` jsdoc tag #4595

Open aruniverse opened 2 years ago

aruniverse commented 2 years ago

Anytime a new api is added, make sure to add the @since jsdoc tag

I think we should probably also document anytime we deprecate an api because its impossible to know in what version something was deprecated w/o going through git history

DanRod1999 commented 1 year ago

I don't know if its possible to use an eslint rule only on new apis. From my understanding, if we want to use a rule that creates a @since tag for us, or just checks if there is one, it would check the entire repo directory where rush lint is called. I've seen ways to use eslint on just changed files here, but then I don't know if there's to run just one rule on these changed files, and the rest of the rules how we normally run them.

pmconne commented 3 months ago

This was marked "in progress" in February of last year. What's happened since?

Anytime a new api is added, make sure to add the @since jsdoc tag

Can you explicitly state the motivation for this new tag?

I think we should probably also document anytime we deprecate an api because its impossible to know in what version something was deprecated w/o going through git history

The @deprecated tag already documents the version in which an API became deprecated. This is enforced through a lint rule and surfaced in our API docs - example. This information seems much more useful and relevant to a consumer of the API than how long the API has existed.

DanRod1999 commented 3 months ago

After my initial research I did not continue working on it since I did not have a reasonable solution at the time. I should have reverted it to something other than "in progress".

As far as if we want to proceed with this, I am not sure. As I mentioned before, I do not know of a way to lint just new apis, and if we require this tag, it would flag all of our apis because none of them currently use @since. There may be a way around this that i'm unaware of, but at the moment I don't know of a way to enforce this rule correctly. I can continue looking into this if we feel it should be included for 5.0, otherwise we can close it. For now reverting to "proposed".

pmconne commented 3 months ago

Thanks. This is indeed the question we need to answer:

if we feel it should be included for 5.0

i.e., what benefit do we expect to get from it and is it worth the effort. @aruniverse?

aruniverse commented 3 months ago

We have 2 sets of consumers:

Apps will always have a direct dependency on itwinjs pkgs, and it makes sense for those teams to always be on the latest versions

However, component pkgs will be using dev + peers, and they should probably be setting their peers to the lowest compatible version an api has been introduced in to have a wider support

I was primarily thinking about those who are writing pkgs when the idea of the @since jsdoc tag came up. I agree it can get messy and theres lot of edge case to consider. Ideally we're not introducing too many new apis in patch releases, and this is mostly something you see for minor releases. And this isn't something youd increment whenever you update something that was previously publicly exported.

I guess we can avoid the since tag if we start documenting nextver.md more explicitly with newly added apis or introduce some other way to let consumers know

aruniverse commented 3 months ago

Adding @raplemie to the convo as we were generally discussing benefits of something like this for civil pkgs last week

raplemie commented 3 months ago

As a package based on iTwin.js developer, one of the benefit I see is that we want to play 2 roles within our packages repo when doing our code.

For our dependency, we want to keep the required package to the lowest version that is required for our package to work, first to reduce the time where our users are forced to update, and second, which I learned lately, if we bump our minimum requirement for peer dependency, this is a breaking change and we are supposed to do that in a major release.

For our development, we try to update the package in our test app/repo pnpm_lock file so we get the latest version, this allows us to validate that new version of the packages we use indeed are still working as expected. However, doing this makes the code completion fill up with potentially new API, that a dev might use without realizing that this has been introduced in a later minor than our package allows it. Having a tag that tells us when the API/method/argument was added would help in this regard, and make us properly update our dependency. (Another option could be for example to have a rush variant build with the earliest version of the supported package, something we did in AppUI for a while...)

ben-polinsky commented 3 months ago

which I learned lately, if we bump our minimum requirement for peer dependency, this is a breaking change and we are supposed to do that in a major release.

Doesn't this effectively disallow us from being able to ~make minor changes~ have our minor changes consumed? Sure, if someone is using core directly they can use the new API, but if the change needs to be consumed by a library to be exposed to applications, it's a no-go?

Edit: it makes sense, just trying to figure out how much this handcuffs us.

raplemie commented 3 months ago

Well, it just means that application will be even more hesitant to consume packages, because they will always see major version bumps. But, yeah, in some sense, it also means that package will try to use less new API because of the added perceived burden of creating/consuming a Major package update.

I guess, since tree shaking has made progress, we could now restitch together the iTwin.js packages to make a single one, and reeingineer the parts that force a single instance of the package to exist. Then we can make iTwin.js a single real dependency, and some of this confusion of updating goes away.

But I know this is not going to happen anytime soon, if ever.

grigasp commented 3 months ago

Well, it just means that application will be even more hesitant to consume packages, because they will always see major version bumps. But, yeah, in some sense, it also means that package will try to use less new API because of the added perceived burden of creating/consuming a Major package update.

I guess, since tree shaking has made progress, we could now restitch together the iTwin.js packages to make a single one, and reeingineer the parts that force a single instance of the package to exist. Then we can make iTwin.js a single real dependency, and some of this confusion of updating goes away.

But I know this is not going to happen anytime soon, if ever.

While bumping a peerDependency is a breaking change, bumping the corresponding devDependency is not. This allows dependent packages to use new APIs if they exist without bumping the peerDependency. Then, on next major version release, the check can be removed. This is definitely a burden on library developers, but IMO it's better than releasing tons of major versions with the only breaking change being the peerDependency bump.

Example of what I mean: https://github.com/iTwin/presentation/blob/master/packages/components/src/presentation-components/common/ContentDataProvider.ts#L407

raplemie commented 3 months ago

True, a since tag would also allow remind devs to do this more easily, if it is clear when a new API is introduced.