fix: read dataset and canvas resources#103
Conversation
🦋 Changeset detectedLatest commit: 488b9c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
cngonzalez
force-pushed
the
fix/sdk-1088/read-dataset-and-canvas-resources
branch
from
71654c4 to
c54e44d
Compare
rdunk
left a comment
rdunk
left a comment
There was a problem hiding this comment.
LGTM, just one small question/comment.
| throw new Error('Dataset clients must include an id in "resource"') | ||
| } | ||
|
|
||
| const [resourceProjectId, resourceDataset] = resource.id.split('.') |
There was a problem hiding this comment.
My only feedback/question would be: is it worth adding extra basic validation here to ensure the ID includes both segments? This handler uniquely depends on the resource ID being a specific shape, so an ID without a . wouldn't produce a "structurally coherent" URL (one with all the segments), whereas the others would and will just 404.
Definitely non-blocking though, just a thought.
There was a problem hiding this comment.
Good call, have added some validation. Thanks!
2 participants
Description
As the SDK moves to using global resources for everything, it would be great for consistency if we pass the same client around to our image builder as well. This PR extends the existing Media Library logic for resources to dataset and canvas resources.
What to review
This should hopefully be pretty straightforward. Are there any places I failed to update?
Testing
Tests were added.