Skip to content

chore(): BREAKING: Remove minore deprecations #10524

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Apr 15, 2025
Merged

chore(): BREAKING: Remove minore deprecations #10524

merged 25 commits into from
Apr 15, 2025

Conversation

asturur
Copy link
Member

@asturur asturur commented Apr 14, 2025 •
edited
Loading

Description

Events

The data duplication between pointer/absolutePointer and viewportPoint/scenePoint has been removed.
Now you find only the latter in the events

Canvas

  • getCenter method has been removed, use only getCenterPoint
  • The internal cache variables _pointer and _absolutePointer have been renamed to _viewportPoint and _scenePoint
  • preserveObjectStacking is now default to true, if you want objects to break the stack when selected, set it to false
  • getPointer deprecated method has been removed, is now a protected _getPointerImpl method.
  • setWidth and setHeight are gone, use only setDimensions

Objects

Text

  • unused method _getSVGLineTopOffset has been deleted

Itext

  • protected _exitEditing is merged with exitEditingImpl public

Image

  • CSS_CANVAS static property from image is gone also with the classList.Add requirement

Utils

  • rotatePoint is gone, use point.rotate instead
  • request is gone. We use fetch, node 18 supports fetch. There is no reason to use the old request object and status
  • setStyle is gone

@codesandbox CodeSandbox
Copy link

codesandbox bot commented Apr 14, 2025

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

@Smrtnyk
Copy link
Contributor

Smrtnyk commented Apr 14, 2025

what about also removing request from util namespace?
In case someone is using it they can move to fetch or their own request implementation

@asturur
Copy link
Member Author

asturur commented Apr 14, 2025

what about also removing request from util namespace? In case someone is using it they can move to fetch or their own request implementation

I left it out because you said you wanted to do it

@Smrtnyk
Copy link
Contributor

Smrtnyk commented Apr 14, 2025

what about also removing request from util namespace? In case someone is using it they can move to fetch or their own request implementation

I left it out because you said you wanted to do it

ah, sure I will implement the load svg to use fetch and then remove the implementation
but I think just removing it from exports on utils would fit more in this PR so it is grouped together

@asturur
Copy link
Member Author

asturur commented Apr 14, 2025

I just deleted stuff, how did i gain so much code image

I must have inverted the new with the old

@asturur
Copy link
Member Author

asturur commented Apr 14, 2025

what about also removing request from util namespace? In case someone is using it they can move to fetch or their own request implementation

I left it out because you said you wanted to do it

ah, sure I will implement the load svg to use fetch and then remove the implementation but I think just removing it from exports on utils would fit more in this PR so it is grouped together

ok good idea i can remove the export right away

@asturur
Copy link
Member Author

asturur commented Apr 14, 2025

to be honest also the util namespace in general is kind of wrong.
It makes hard to codesplit nicely when you import a single util.

@Smrtnyk
Copy link
Contributor

Smrtnyk commented Apr 14, 2025

to be honest also the util namespace in general is kind of wrong. It makes hard to codesplit nicely when you import a single util.

yep
maybe we need a new entry in package.json exports like fabric/util?

@asturur
Copy link
Member Author

asturur commented Apr 14, 2025

I think so, that was already discussed in the past, but i m not going to touch it now.

@fabricjs fabricjs deleted a comment from github-actions bot Apr 14, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 14, 2025 •
edited
Loading

Build Stats

file / KB (diff) bundled minified
fabric 913.863 (-3.149) 301.232 (-0.613)

Smrtnyk
Smrtnyk reviewed Apr 14, 2025
View reviewed changes
src/util/index.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like request is back on the utils?
but got removed in prior commit 40236e4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes because old tests are using it and i don't want this pr to address that too.

Smrtnyk
Smrtnyk reviewed Apr 15, 2025
View reviewed changes
src/benchmarks/cloneDeep.mjs
@@ -4,5 +4,4 @@
* @param object
* @returns
*/
export const cloneDeep = <T extends object>(object: T): T =>
JSON.parse(JSON.stringify(object));
export const cloneDeep = (object) => JSON.parse(JSON.stringify(object));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this implementation needed?
there is lodash in the project which should offer this same functionality

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those benchmark are just here for historic memory of why we removed some code with other.
When we found deep performance difference we changed and we left a simple script to document the decision.
That is why i keep a copy of it there. Just in case someone come and wants to use a deep clone logic again instead of a specialized cloning for the data structure

@github-actions GitHub Actions
Copy link
Contributor 


> fabric@6.6.2 coverage:report:ci
> nyc report --reporter=text-summary


=============================== Coverage summary ===============================
Statements   : 72.89% ( 20943/28732 )
Branches     : 59.87% ( 3843/6418 )
Functions    : 87.33% ( 1572/1800 )
Lines        : 77.4% ( 16033/20712 )
================================================================================

@asturur
Copy link
Member Author

asturur commented Apr 15, 2025

I ll merge this.
I think is a decent cleanup but not a massive one that scares people off migrating.

@asturur asturur merged commit 25e42f8 into master Apr 15, 2025
21 checks passed
@asturur asturur deleted the remove-deprecated branch April 15, 2025 13:55
@asturur asturur mentioned this pull request Apr 17, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@Smrtnyk Smrtnyk Smrtnyk left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asturur @Smrtnyk