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

Handle Empty SVGs #117

Merged
merged 2 commits into from
Apr 24, 2022
Merged

Conversation

amcolash
Copy link

I have been having an issue where the Figma API is returning empty SVG files. I am not sure if this is a common problem or if there is a known solution. Either way, I submitted a ticket to Figma support for help on the issue.

In the meantime, I think that handling the invalid file earlier rather than later gives a much better debugging experience instead of having the parsing of the empty SVG file happen later on.

This PR handles such a case more gracefully.

This is my first time submitting code to this repo - not sure if each change is a separate version or if you bundle your changes together. Happy to do anything else needed! Thanks :)

@marcomontalbano
Copy link
Owner

Hi @amcolash, thanks a lot for your contribution! I think that handling this issue on this side is fine while waiting for an answer from Figma.

Just realized that CONTRIBUTING.md is the wrong place where putting information about release process, this could be misleading. May I ask you to remove your latest commit about version bump? Usually this is something I manage, as you said, bundling changes together.

I'll move the release process in a different file, and update the CONTRIBUTING.md. Thank to spotted this out 😄

@marcomontalbano marcomontalbano added the PR: Bug Fix 🐛 Only for pull request. Something wasn't working label Apr 20, 2022
@amcolash
Copy link
Author

amcolash commented Apr 20, 2022

No worries at all, I have reverted the versions.

Upon further investigation today, I have found that the API is not "technically" broken. There was an empty frame and the API correctly returned an empty file.

From my work on my exporting project, it has been an ongoing issue. Some versions of this file just have empty frames, which might be part of my team's design process.

I asked support if their engineers would consider changing the API to instead return an empty svg file.
Regardless of how figma support responds, I can think of a few different options in this scenario:

  1. Keep original PR and throw an error saying that all components need svg - bad for automation and long-term stability.
  2. Fill that component with a small empty svg string: <svg></svg> - odd, but it makes sure everything works as expected. Might be worth adding in a width/height/viewbox if it can be inferred.
  3. Entirely skip the file - might be tricky + unclear why components are not exported.

Of the three options, I prefer 2 since everything is exported as expected. The empty svg files are harmless in my opinion and are reflective of the state of the component at that point. We could add in a warning message, so at least it is logged that the component is empty during the export.

Here is an idea of how option 2 could look.

if (response.data.length === 0) return '<svg></svg>';
return response.data;

OR a slightly more verbose option:

if (response.data.length === 0) return '<svg xmlns="http://www.w3.org/2000/svg"></svg>';
return response.data;

Thoughts?

@amcolash amcolash force-pushed the empty-svg branch 2 times, most recently from 2a1274a to 8a3098d Compare April 20, 2022 20:32
@amcolash
Copy link
Author

Quick update: Got a response from figma support - it sounds like this may be intended behavior, so I think handling it in the export is probably the best option for now.

@marcomontalbano
Copy link
Owner

Hi @amcolash, thanks for this analysis.
About the empty svg I'm agree with you, I think that is much better than throwing an error, since the workflow could run in the moment in which you are creating a new component and this will break the execution.
I would go with the shorter <svg></svg> since there's no content.

I'm going to accept this PR and bundle with the next release!

Thanks!

@marcomontalbano marcomontalbano merged commit 1aa12df into marcomontalbano:master Apr 24, 2022
@amcolash
Copy link
Author

Awesome, thanks so much! Appreciate the help and happy to contribute to this great project 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 Only for pull request. Something wasn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants