Download blob: remove downloadjs dependency #56024
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
[Type] Enhancement
ramonjd
requested review from
swissspidy,
glendaviesnz,
kevin940726 and
aaronrobertshaw
|
Size Change: -870 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
try/remove-downloadjs-dependency
branch
from
7ee6226
to
41fd20e
Compare
|
Flaky tests detected in 1499a6a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6847000101
|
gziolo
reviewed
Nov 10, 2023
gziolo
added
[Type] New API
Thanks for the feedback! What was the concern exactly? I wasn't very sure about the function name either 🤔 |
kevin940726
reviewed
Nov 13, 2023
packages/blob/src/index.js
Outdated
| a.click(); | ||
| document.body.removeChild( a ); | ||
| window.URL.revokeObjectURL( url ); | ||
| return a; |
There was a problem hiding this comment.
Any reason why we want to return the anchor element instead of removing it to avoid memory leak?
There was a problem hiding this comment.
Thanks for reviewing!
By "removing" do you mean passively via garbage collection, or something other than removing from DOM?
I returned for two reasons:
- To have something the tests can easily verify
- Just in case consumers what to do something with a link element
Reason 2 is not a very good reason though 😄
There was a problem hiding this comment.
I feel like the anchor element is an implementation detail and shouldn't be exposed to the consumers. What the consumers expect is just a function which triggers downloading something. The same goes with testing, we should avoid testing implementation details if possible (probably a bit trickier here 😅). If the user keeps the returned element then it could be a source of memory leak (though doesn't seem that impactful).
There was a problem hiding this comment.
I agree. I'll remove. Thanks!
There was a problem hiding this comment.
The same goes with testing, we should avoid testing implementation details if possible (probably a bit trickier here)
You were right, but I think managed to hack together a replacement test 🙃
There was a problem hiding this comment.
Yeah I guess it doesn't really matter here if we have a solid unit test. Whatever method we end up with will have to rely on mocking implementation details in jsdom. A potential better method would be doing it in integration testing with a real browser for instance, but that's out of the scope and probably not worth it here.
Removes downloadjs dependency Adds tests
talldan
reviewed
Nov 13, 2023
- changed a var to anchorElement - regenerate docs
ramonjd
force-pushed
the
try/remove-downloadjs-dependency
branch
from
8c48717
to
ef6e22e
Compare
gziolo
reviewed
Nov 13, 2023
| * @param {string} contentType File mime type. | ||
| */ | ||
| export function downloadBlob( filename, content, contentType ) { | ||
| const file = new window.Blob( [ content ], { type: contentType } ); |
There was a problem hiding this comment.
Can we add some validation to the parameters passed?
Should at least a default mime type be defined in case nothing is passed? Will it work correctly if you pass the incorrect content type?
It's going to be a general purpose util available also outside the WordPress context.
There was a problem hiding this comment.
Can we add some validation to the parameters passed?
Yes, good idea. Thanks!
Should at least a default mime type be defined in case nothing is passed? Will it work correctly if you pass the incorrect content type?
I'll test this. Maybe text/plain could work.
It might be that a default mime-type might be incompatible with content<any> 🤔 so in that case we could require all the args.
There was a problem hiding this comment.
Will it work correctly if you pass the incorrect content type?
I've tested with a fallback content type of text/plain and things like zips and images are still downloadable, and the browser somehow works it out.
However, type is an optional parameter and the default value is "", so I think we can can fallback to "" and require the other args.
…I spec for Blob Updates tests
|
@gziolo @kevin940726 Thank you for helping with this PR 🙇🏻 @gziolo As mentioned above, I've created a fallback for
If you're happy with the changes, then I'll merge this one tomorrow. Thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR is a follow up to:
Here's what it does:
downloadBlob— in the@wordpress/blobpackage to handle file downloads.downloadfunctionsWhy?
As demonstrated in #55912, the downloadjs package contained some bugs in relation to non-ASCII characters.
The dependency was also not required to handle site exports.
How?
Moves an existing function in reusable blocks to the
@wordpress/blobpackage.Testing Instructions
Site editor
Patterns
Reusable blocks
The new function came from here and is basically the same, with the exception that, now, it's pulled from the blob package.
Create a pattern and head over to
/wp-admin/edit.php?post_type=wp_blockExport a pattern as JSON.