Skip to content

Deprecate extension methods returning Point #164

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

Merged

Conversation

parlough
Copy link
Member

@parlough parlough commented Feb 7, 2024

Point really shouldn't exist in its current form so we shouldn't encourage or enable its use.

If anyone still thinks these extensions are helpful, maybe we could return a record instead.

@kevmoo
Copy link
Member

kevmoo commented Feb 7, 2024

Ooo....record could be nice.

@kevmoo
Copy link
Member

kevmoo commented Feb 7, 2024

@srujzs @devoncarew – thoughts here?

@kevmoo
Copy link
Member

kevmoo commented Feb 7, 2024

Why don't we like Point, @parlough ?

@srujzs
Copy link
Contributor

srujzs commented Feb 7, 2024

I am curious why we want to discourage Point as well - is dart:math planning on deprecating it?

@parlough
Copy link
Member Author

parlough commented Feb 7, 2024

A Point on a screen does not really belong in a math library, nor does Rectangle. Beyond that, Point declares operators that don't make sense for a point, conflating it with a vector (which would make more sense). The * operator on Point<int> can also have a runtime error 😢 when provided with a double.

Beyond those potential sources of confusion, I guess it being generic can introduce performance issues (dart-lang/sdk#53912).

is dart:math planning on deprecating it?

I believe there's been a variety of other discussions around this over the years, but since dart:html used it, it was likely hard to deprecate and remove. With this web migration, I think it's time to revisit that. Whether we do/can or not, it seems better avoid new uses where possible. Anyone using Point here likely just needs the x and y, and direct access or a record can provide that.

Edit: Formalized this into an SDK issue: dart-lang/sdk#54852

@srujzs
Copy link
Contributor

srujzs commented Feb 8, 2024

Thanks for the reasoning! I'm okay with avoiding new uses for now. We can always re-add (or users can in their own libraries) if we think this is safe.

@srujzs srujzs self-requested a review February 8, 2024 17:26
@kevmoo
Copy link
Member

kevmoo commented Feb 8, 2024

Or do we just move to the Record and include it in the breaking change?!?!

Might be good to do now!

@srujzs
Copy link
Contributor

srujzs commented Feb 8, 2024

Either way works. I presume there's very low usage of this, and since we're releasing a breaking change version, might as well remove it instead of deprecating.

@kevmoo
Copy link
Member

kevmoo commented Feb 8, 2024

@srujzs – or just replace the return type!

@parlough
Copy link
Member Author

parlough commented Feb 8, 2024

I'd really suggest we deprecate or remove these for this release. Then revisit extensions returning records in the future. For example MouseEvent has a bunch of other somethingX and somethingY values that we don't expose through extensions like this. Seems better to have a consistent experience until its decided to expose these extensions consistently.

It's much easier to add extensions back :)

@srujzs srujzs merged commit 7bc75c3 into dart-lang:main Feb 12, 2024
@parlough parlough deleted the misc/deprecate-point-extension-methods branch February 12, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants