Adding BlobVar and BytesVar.
#5985
Conversation
CodSpeed Performance ReportMerging #5985 will not alter performanceComparing Summary
|
Greptile Summary
Important Files Changed
Confidence score: 4/5
Sequence DiagramsequenceDiagram
participant User
participant StringVar
participant BytesVar
participant BlobVar
participant LiteralBytesVar
participant LiteralBlobVar
User->>StringVar: "encode('utf-8')"
StringVar->>StringVar: "string_encode_operation()"
StringVar-->>BytesVar: "return BytesVar"
User->>BytesVar: "create(b'data')"
BytesVar->>LiteralBytesVar: "create(bytes_value)"
LiteralBytesVar->>LiteralBytesVar: "new Uint8Array([...])"
LiteralBytesVar-->>BytesVar: "return BytesVar"
User->>BytesVar: "decode('utf-8')"
BytesVar->>BytesVar: "bytes_decode_operation()"
BytesVar-->>StringVar: "return StringVar"
User->>StringVar: "to_blob('text/plain')"
StringVar->>BlobVar: "blob_object_create_operation()"
BlobVar->>BlobVar: "new Blob([data], {type: mime_type})"
BlobVar-->>BlobVar: "return BlobVar"
User->>BlobVar: "create_object_url()"
BlobVar->>BlobVar: "create_url_from_blob_operation()"
BlobVar->>BlobVar: "window.URL.createObjectURL()"
BlobVar-->>StringVar: "return URL string"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 files reviewed, 4 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
try to reduce the redundant paths in the new code. there should never be two separate paths to format what is essentially the same value; the Var system is cumulative, so if you have already correctly dealt with some subtype in a different Var, you can just include that directly in other expressions.
pay attention to the diference between the base type Var which corresponds to some JS-native type (StringVar, BytesVar, ObjectVar, etc) and the LiteralVar which corresponds to some Python-native type. The LiteralVar is responsible for representing the python value in the JS world, and the base type Var is responsible for defining what operations on that type can be performed in JS on the frontend.
it would be helpful to add some test case, at the very least a few more examples in tests/integration/test_var_operations.py where these various conversions are used end-to-end in the browser to validate their functionality, but more importantly to notice when some future change in reflex causes their behavior to regress.
| A StringVar containing the decoded string. | ||
| """ | ||
| return var_operation_return( | ||
| f"(new TextDecoder({encoding})).decode(new Uint8Array({value}))", |
There was a problem hiding this comment.
rx.Var.create(b"foobar").decode()
Produces code that duplicates the Uint8Array (it still works, but it seems suboptimal)
(new TextDecoder("utf-8")).decode(new Uint8Array(new Uint8Array([102, 111, 111, 98, 97, 114])))
| @classmethod | ||
| def create( | ||
| cls, value: str | bytes, _var_data: VarData | None = None | ||
| ) -> LiteralBytesVar: | ||
| """Create a BytesVar from a string or bytes value. | ||
| Args: | ||
| value: The string or bytes value to create the var from. | ||
| _var_data: Additional hooks and imports associated with the Var. | ||
| Returns: | ||
| A LiteralBytesVar representing the bytes value. | ||
| """ | ||
| if isinstance(value, str): | ||
| value = value.encode() | ||
| return LiteralVar.create(value, _var_data=_var_data) |
There was a problem hiding this comment.
The non-literal BytesVar should not define create like this -- that's what the LiteralBytesVar is for.
The non-literal Var just holds the valid operations for that type, and generally speaking the _js_expr will be some variable name (imagine a State variable of type bytes, the _js_expr would be the state+var name).
| @classmethod | ||
| def create( | ||
| cls, | ||
| value: str | bytes | Blob | Var, | ||
| mime_type: str | Var | None = None, | ||
| _var_data: VarData | None = None, | ||
| ): | ||
| """Create a BlobVar from the given value and MIME type. | ||
| Args: | ||
| value: The data to create the Blob from (string, bytes, or Var). | ||
| mime_type: The MIME type of the Blob (string or Var). | ||
| _var_data: Optional variable data. | ||
| Returns: | ||
| A BlobVar instance representing the JavaScript Blob object. | ||
| """ |
There was a problem hiding this comment.
Combine this with LiteralBlobVar.create and remove the definition in the non-literal BlobVar
| value: bytes | str | Blob, | ||
| mime_type: str | None = None, |
There was a problem hiding this comment.
i think this method should just take a Blob object to simplify the API.
it also needs to assign that Blob object to _var_value so it can be pulled out on the python side later.
| @classmethod | ||
| def _determine_mime_type(cls, value: str | bytes | Blob | Var) -> str: | ||
| mime_type = "" | ||
| if isinstance(value, str | bytes | Blob): | ||
| match value: | ||
| case str(): | ||
| mime_type = "text/plain" | ||
| case bytes(): | ||
| mime_type = "application/octet-stream" | ||
| case Blob(): | ||
| mime_type = value.mime_type | ||
|
|
||
| elif isinstance(value, Var): | ||
| if isinstance(value._var_type, str): | ||
| mime_type = "text/plain" | ||
| if isinstance(value._var_type, bytes): | ||
| mime_type = "application/octet-stream" | ||
|
|
||
| if not mime_type: | ||
| msg = "Unable to determine mime type for blob creation." | ||
| raise ValueError(msg) | ||
|
|
||
| return mime_type |
There was a problem hiding this comment.
suggest moving this logic into a create function for the Blob dataclass
| if isinstance(value._var_type, bytes): | ||
| value = f"new Uint8Array({value})" |
There was a problem hiding this comment.
if the value is a BytesVar, then you don't need this additional conversion, because the BytesVar already includes the array cast in its _js_expr, so when you format it into the expression, it will already appear like this
str(rx.Var(f"{rx.Var.create(b"foo")}")) -> 'new Uint8Array([102, 111, 111])'
| if isinstance(value, BytesVar): | ||
| return var_operation_return( | ||
| js_expression=f"new Blob([new Uint8Array({value})], {{ type: {mime_type} }})", | ||
| var_type=Blob, | ||
| ) |
There was a problem hiding this comment.
this case is unnecessary; if the value is a BytesVar, then simply formatting it into the js_expression below will use it's JS-form which already includes the Uint8Array cast
2 participants
Provides better support for converting Var data to JS Blobs. Adds
BytesVarto back Pythonbyteswith JSUint8Array. To keep things consistent and pythonic,.encode(...)and.decode(...)methods have been added toStringVar. Also, bothStringVarandBytesVarhave ato_blob(...)method that assists in creating anew Blob(...)from the Python data. TheBlobVarhas a.create_object_url(...)method that returns the URL string for downloading withrx.download(...). It also seems to handle Unicode quite nicely.All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features: