fix: exporting labelled arrows via export utils

[ad1992]

fixes #6419

Approach 1

The issue was since export utilities are pure JS functions hence trying to extract bound text elements/text containers via Scene will not work. So I have added an optional param sceneElements to the methods getBoundTextElement and getContainerElement which if passed will use these elements instead of scene.
The sceneElements will be passed only in case of export

Approach 2

I am initialising the scene in the utils instead so its available to the helper methods and wherever its called from outside

Export-labels.mp4

TODOS

• Update changelog
• Fix library rendering - it has the same bug

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
excalidraw	✅ Ready (Inspect)	Visit Preview	Apr 13, 2023 11:33am
excalidraw-package-example	✅ Ready (Inspect)	Visit Preview	Apr 13, 2023 11:33am

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
docs	⬜️ Ignored (Inspect)		Apr 13, 2023 11:33am

[dwelle]

The proposed solution isn't type-safe. And what about all the other applicable helpers that are either currently using or will potentially use Scene in the future? We need to come up with a safe solution.

So either remove Scene getters from all helpers that should be pure, or ensure Scene is pre-populated before using those helpers.

I was already thinking about the former a bit. We should have pure helpers that can be used anywhere, and we should have the editor instance curry those helpers for "everyday" and public API use.

The problem is that the Scene case is specific in that we can't simply replace it with elements array because that has O(n) complexity unlike O(1) when using Scene.getScene(element).getElement().

[ad1992]

The proposed solution isn't type-safe. And what about all the other applicable helpers that are either currently using or will potentially use Scene in the future? We need to come up with a safe solution.

So either remove Scene getters from all helpers that should be pure, or ensure Scene is pre-populated before using those helpers.

I was already thinking about the former a bit. We should have pure helpers that can be used anywhere, and we should have the editor instance curry those helpers for "everyday" and public API use.

The problem is that the Scene case is specific in that we can't simply replace it with elements array because that has O(n) complexity unlike O(1) when using Scene.getScene(element).getElement().

Yeah I thought about the same, I agree we should have pure helpers but thinking about the time complexity, I added an optional param which will gain more preference if passed. For utilities at least we need it to be pure functions.
As for the fix targeted in this PR - are you suggesting to replace Scene with elements array ?

[dwelle]

Yeah I thought about the same, I agree we should have pure helpers but thinking about the time complexity, I added an optional param which will gain more preference if passed. For utilities at least we need it to be pure functions.
As for the fix targeted in this PR - are you suggesting to replace Scene with elements array ?

We shouldn't use optional params like this, but I wouldn't even make it non-optional because it's unclear when and when not we should pass it. By not clear I mean it requires knowledge of the implementation details, which may also change any moment, so it's very unsafe.

If we're to hack this, which in the context of this PR it seems we'll do either way, then I suggest we prepopulate the Scene cache inside the export and other applicable helpers.

There are a few caveats to this though:

1. we shouldn't put it into helpers that we call ourselves within the editor (if that's the case)
2. could result in a memory leak
3. could be a performance issue even if used only in the context of programmatic exports, if the host app uses it in a tight loop somewhere

I'm not happy with the above suggestion either.

We might wanna think on how to solve this properly.

[ad1992]

Yeah I thought about the same, I agree we should have pure helpers but thinking about the time complexity, I added an optional param which will gain more preference if passed. For utilities at least we need it to be pure functions.
As for the fix targeted in this PR - are you suggesting to replace Scene with elements array ?

We shouldn't use optional params like this, but I wouldn't even make it non-optional because it's unclear when and when not we should pass it. By not clear I mean it requires knowledge of the implementation details, which may also change any moment, so it's very unsafe.

If we're to hack this, which in the context of this PR it seems we'll do either way, then I suggest we prepopulate the Scene cache inside the export and other applicable helpers.

There are a few caveats to this though:

1. we shouldn't put it into helpers that we call ourselves within the editor (if that's the case)
2. could result in a memory leak
3. could be a performance issue even if used only in the context of programmatic exports, if the host app uses it in a tight loop somewhere

I'm not happy with the above suggestion either.

We might wanna think on how to solve this properly.

This doesn't sound like a good approach :(. Instead I would prefer to add the logic of extracting the container/bound element in those specific utilities instead of calling the helper functions and also decouple the masking part so it can be used independently if we don't agree on optional param. But that would be more code change / effort which would be removed soon.
So I think we can still merge this PR so fix is available and come up with a better solution in a separate PR (as this is one of the issues which has caused a couple of bugs in the past as well be it in app/package so we would want to fix it the right way which might take some time) as thats into going to impact the host in any way.

[dwelle]

So I think we can still merge this PR so fix is available and come up with a better solution in a separate PR (as this is one of the issues which has caused a couple of bugs in the past as well be it in app/package so we would want to fix it the right way which might take some time) as thats into going to impact the host in any way.

No, because 1) it's not a full fix (it just randomly fixes a few cases) and is unsafe, and 2) it adds complexity and technical debt.

Instead I would prefer to add the logic of extracting the container/bound element in those specific utilities instead of calling the helper functions and also decouple the masking part so it can be used independently if we don't agree on optional param. But that would be more code change / effort which would be removed soon.

If you're suggesting factoring out the container retrieval outside of the helpers, then it makes sense only for functions that deal with a single element. E.g. while rendering, we'd still have to retrieve the container from somewhere, and we need it to be O(1) (which btw this PR makes it an an O(n) even for editor purposes, which is bad). So we'd have to implement our own precaching inside the renderScene/renderElement (which would still not be great if we were to do it on each renderScene() call).

[samiraluee]

Hi, I am using your npm package and on version 14 I can draw an arrow with text element on it with no issue and export to svg with no issues, after upgrading to version 15 I can no longer open svg files created by version 14 where there is an arrow with text element in the svg. also when drawing arrow with text element on it and try to export to svg , excalidraw component crashes my application. the only change I made to my application is upgrading excalidraw from v14 to v15. could you please help me understand what's going wrong? If is this a known bug, do you have a timeline to fix it? Thanks for your help.