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

chore: deprecate flat #165

Merged
merged 4 commits into from
Aug 16, 2024
Merged

chore: deprecate flat #165

merged 4 commits into from
Aug 16, 2024

Conversation

MarlonPassos-git
Copy link
Contributor

@MarlonPassos-git MarlonPassos-git commented Aug 10, 2024

Tip

The owner of this PR can publish a preview release by commenting /publish in this PR. Afterwards, anyone can try it out by running pnpm add radashi@pr<PR_NUMBER>.

Summary

Update the JSDOC and documentation about the function being deprecated.

Related issue, if any:

https://github.com/orgs/radashi-org/discussions/84

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed
  • Related benchmarks have been added or updated, if needed

Does this PR introduce a breaking change?

Yes

Bundle impact

Status File Size 1 Difference (%)
M src/array/flat.ts 49 -28 (-36%)

Footnotes

  1. Function size includes the import dependencies of the function.

@MarlonPassos-git MarlonPassos-git changed the title docs(flat): deprecate function docs(flat)!: deprecate function Aug 10, 2024
@aleclarson aleclarson added the BREAKING CHANGE Not backwards compatible label Aug 14, 2024
@aleclarson aleclarson changed the base branch from main to next August 14, 2024 20:49
@aleclarson
Copy link
Member

Debating whether to merge this into the next minor or major version. 🤔

The SemVer spec states the following:

How should I handle deprecating functionality?

Deprecating existing functionality is a normal part of software development and is often required to make forward progress. When you deprecate part of your public API, you should do two things: (1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

But, considering how VSCode does a "strike-through" of deprecated APIs, I wonder if saving deprecations for a major version isn't a more user-friendly policy.

Any thoughts, @radashi-org/core?

@MarlonPassos-git
Copy link
Contributor Author

MarlonPassos-git commented Aug 14, 2024

Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

I prefer a minor release for the @deprecated in JSDOC and a major release for code remotion.
We can pass a long time to remove something if we wait for one major release to alert us about deprecation and another major release to remove.

@aleclarson aleclarson changed the base branch from next to main August 16, 2024 01:40
@radashi-bot
Copy link

radashi-bot commented Aug 16, 2024

Benchmark Results

Name Current Baseline Change
flat ▶︎ with non-empty input list 835,525.32 ops/sec ±0.18% 3,440,316.31 ops/sec ±0.19% 🔗 🐢 -75.71%

Performance regressions of 30% or more should be investigated, unless they were anticipated. Smaller regressions may be due to normal variability, as we don't use dedicated CI infrastructure.

@aleclarson aleclarson changed the title docs(flat)!: deprecate function chore: deprecate flat Aug 16, 2024
@aleclarson aleclarson merged commit d1bc75a into radashi-org:main Aug 16, 2024
9 checks passed
Copy link

A new beta version 12.2.0-beta.6b76988 has been published to NPM. 🚀

To install:

The radashi@beta tag also includes this PR.

See the changes

@aleclarson
Copy link
Member

aleclarson commented Aug 16, 2024

Note: I replaced the old _.flat implementation with one that uses Array.prototype.flat instead, since they are equivalent behavior.

But somehow, I read the benchmark results as a +75% improvement, so I merged prematurely. Sadly, using Array.prototype.flat appears to be about 75% slower than the Radash implementation.

That's quite surprising, considering that Array.prototype.flat is a native method. 🤔

I wonder if we should revert the deprecation or should we assume that (1) Array.prototype.flat will get faster in time and (2) ~800K ops/sec is fast enough for most use cases. Thoughts, @radashi-org/core?

edit: I should note that the benchmark for _.flat includes the allocation of the array of nested arrays (see here). So 800K ops/sec is not actually how slow Array.prototype.flat is. Nonetheless, the performance gap between the Radash implementation and Array.prototype.flat is still accurate.

@aleclarson
Copy link
Member

aleclarson commented Aug 16, 2024

I'm gonna have to revert this, because Array.prototype.flat was added in ES2019, and we're going to support browsers as old as ES2017 (without legacy transforms).

I thought eslint-plugin-compat would catch this, but it (sadly) doesn't actually check method usage: amilajack/eslint-plugin-compat#258

I'm in the process of updating our tsconfig.json to use "es2017" instead of "esnext" so we can catch these issues with our pnpm lint script. I chose ES2017 because that's when async-await syntax was introduced.

aleclarson added a commit that referenced this pull request Aug 16, 2024
Radashi supports browsers as old as ES2017, so `Array.prototype.flat` is not a viable alternative.

This reverts commit d1bc75a.
@aleclarson aleclarson added yanked This PR was yanked before publishing and removed BREAKING CHANGE Not backwards compatible labels Aug 16, 2024
@MarlonPassos-git
Copy link
Contributor Author

What do you mean by keeping jsdoc saying that there is currently a native implementation in ES2019?

@aleclarson
Copy link
Member

@MarlonPassos-git That's a good idea! PR welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yanked This PR was yanked before publishing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants