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

Assets endpoints are missing CLI transport #211

Open
tommoor opened this issue Jan 27, 2020 · 7 comments
Open

Assets endpoints are missing CLI transport #211

tommoor opened this issue Jan 27, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@tommoor
Copy link
Contributor

tommoor commented Jan 27, 2020

This will soon become a blocker for partial sync. Currently asset download on web / desktop work quite differently – perhaps this is an opportunity to bring them more inline?

Related #84

@tommoor tommoor added the enhancement New feature or request label Jan 27, 2020
@tommoor
Copy link
Contributor Author

tommoor commented Feb 18, 2020

Ping on this one.

@bitpshr
Copy link
Contributor

bitpshr commented Feb 18, 2020

The quickest path to get something live here is to handle this in the same way files are currently handled - the API transport would expose the ability to work with the resulting ArrayBuffer, whereas the CLI transport would not. Beyond this type of an asymmetric approach that uses existing CLI functionality, we'd have to work out streaming mechanics in CLI land to match API behavior, which would definitely slow this down.

Is the API consolidation a firm requirement? If not, we can get this live pretty quickly.

@bitpshr bitpshr self-assigned this Feb 18, 2020
@tommoor
Copy link
Contributor Author

tommoor commented Feb 19, 2020

I think the API consolidation might be a requirement, otherwise we'd have to do that in the UI codebase – the desktop app would have to know if the assets were coming from the API or CLI and act accordingly.

@bitpshr
Copy link
Contributor

bitpshr commented Feb 20, 2020

Couldn't the desktop application call assets.file({ ... }, { transportMode: ['cli']}), which would mimic the same native CLI functionality today, e.g. saving a file to a location on disk? Is partial sync introducing new application behavior that requires the CLI to actually stream back asset responses so they can be manipulated in some way?

I think I'm lacking context: since web uses API asset download and desktop uses CLI asset download today and things work fine, why do we need to consolidate the underlying semantics of how the API vs. CLI work to move these to SDK requests, especially if application behavior isn't changing?

@tommoor
Copy link
Contributor Author

tommoor commented Feb 20, 2020

Partial sync means that the assets will not always be on disk, especially for the usecase of a developer attempting to download assets from a designers branch – they are unlikely to have the project cloned/synced locally – in this case we'll need to fall back to the API transport in desktop.

@bitpshr
Copy link
Contributor

bitpshr commented Feb 21, 2020

Sorry to push the point, but couldn't desktop clients use the already-existing API asset endpoint in the case described above in which an asset is not local? E.g. where a desktop client currently calls into the CLI directly to save a local asset, it could instead call sdkClient.assets.file({ ... }, { transportMode: ['cli', 'api']}), first trying the CLI for the local file, then falling back to the API if the CLI fails. Does some other part of the new partial sync flow require access to the streamed response? If not, we should be OK just exposing what we already have, letting the API stream a response back while the CLI does not.

I agree that we should consolidate the semantics of the API and CLI at some point, but I still don't see how this is a blocker or why we shouldn't at least expose current CLI asset behavior to unblock partial sync / the scenario described above.

@tommoor
Copy link
Contributor Author

tommoor commented Feb 24, 2020

Does some other part of the new partial sync flow require access to the streamed response? If not, we should be OK just exposing what we already have, letting the API stream a response back while the CLI does not.

No I don't think so – as long as we can use both transports to save the resulting asset to a specific location on disk with the same interface that's all that should be needed.

@bitpshr bitpshr removed their assignment Feb 27, 2020
@berezovskyicom berezovskyicom self-assigned this Mar 19, 2020
@berezovskyicom berezovskyicom removed their assignment Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants