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

Client-side API #353

Closed
wants to merge 23 commits into from
Closed

Client-side API #353

wants to merge 23 commits into from

Conversation

surma
Copy link
Collaborator

@surma surma commented Nov 29, 2018

Here’s the current, rather hacky implementation of the client-side API so we can start discussing the implementation approach.

The basic approach is to define an API and expose it to window.parent via Comlink, this way squoosh can be embedded using <iframe> and the API methods can just be invoked.

Here’s the code for an example batch processing app.

src/components/App/index.tsx Outdated Show resolved Hide resolved
@jakearchibald
Copy link
Collaborator

This is a 12% increase to our first-interaction bundle. Is there anything that can be done to avoid this?

What does usage look like? How does the user know when they can make calls to the API?

@surma
Copy link
Collaborator Author

surma commented Dec 3, 2018

@jakearchibald PTAL! Lazy-loading the API now :)

I also updated the example batch app, which actually looks really nice in terms of code now. I’d love to bundle squoosh.api.js with our project and serve it as its own bundle from squoosh.app, but I couldn’t get webpack to do that lol.

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming along nicely. I agree that we should use this for the share target stuff.

src/components/App/index.tsx Outdated Show resolved Hide resolved
src/components/App/index.tsx Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
@jakearchibald
Copy link
Collaborator

We need to decide if the API is solely for computation, or if it's a UI driver. From this PR it seems like the latter, and I think that's the right direction. That works for things like a share target API, it works for creating a glitch that opens a particular image with particular settings (once we expand the API to include settings). But that means the API needs to cope with user interactions that might happen during its driving, so calls should AbortError if the user interrupts.

I don't have a strong opinion if this is done with events, or promises stored on the instance.

Some things to think about:

If the user changes some compression settings on the left, then the API changes settings on the right, should the API returned promise wait on both sides to finish compressing, or just the right?

If this is a UI driver, should we separate the setting of things from the completion of things? Eg, if I setFile, should that resolve once the image has been set from the UI point of view, as in before it's decoded? We could have other methods for observing things like decoded, and compression complete.

What's our backwards compatibility story here? Should the API involve some sort of version check? We could change our major version when the API breaks compatibility.

@surma
Copy link
Collaborator Author

surma commented Dec 7, 2018

We need to decide if the API is solely for computation, or if it's a UI driver. From this PR it seems like the latter,

The more I think about it, the more I agree it’s what we should aim for.

But that means the API needs to cope with user interactions that might happen during its driving, so calls should AbortError if the user interrupts.

I’ll see if I can add a got way to add that.

If the user changes some compression settings on the left, then the API changes settings on the right, should the API returned promise wait on both sides to finish compressing, or just the right?

I don’t think it should. setFile() just waits for the state change to happen. getBlob(side) waits for the selected side to finish encoding. This is where I’d need to add error handling for when that job gets aborted. I think that model works.

If this is a UI driver, should we separate the setting of things from the completion of things? Eg, if I setFile, should that resolve once the image has been set from the UI point of view, as in before it's decoded? We could have other methods for observing things like decoded, and compression complete.

It’s mostly how it works right now, but I think you are right that more explicit events (?) for intermediate steps would be nice.

What's our backwards compatibility story here? Should the API involve some sort of version check? We could change our major version when the API breaks compatibility.

It’s probably a good call to add a the major version to the handshake and just abort on mismatch.

@jakearchibald
Copy link
Collaborator

jakearchibald commented Dec 7, 2018

but I think you are right that more explicit events (?) for intermediate steps would be nice.

I'm leaning towards promises being better, but I'm not awfully sure. After driving the UI, you could get a promise for "image decoded" (which is when you could get width/height & format if we offered it), and a promise for the complete encoding of a particular side.

Edit: When I say "get a promise", I mean it'd be an additional call to get that promise. The methods themselves would resolve once the state is set.

It’s probably a good call to add a the major version to the handshake and just abort on mismatch.

Error I think.

@surma
Copy link
Collaborator Author

surma commented Dec 17, 2018

@jakearchibald PTAL

src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
@surma
Copy link
Collaborator Author

surma commented Dec 19, 2018

@jakearchibald PTAL

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caught a few more things.

Also, there should be docs for this, which we can link to from the main README.

src/components/compress/index.tsx Outdated Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
src/components/compress/index.tsx Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
src/components/App/client-api.ts Show resolved Hide resolved
src/components/compress/index.tsx Outdated Show resolved Hide resolved
@surma
Copy link
Collaborator Author

surma commented Feb 11, 2019

@jakearchibald PTAL

@jakearchibald jakearchibald mentioned this pull request Feb 19, 2019
Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nits.

Add some docs and a demo, and we're good to go I think.

src/components/App/client-api.ts Show resolved Hide resolved
src/components/App/client-api.ts Outdated Show resolved Hide resolved
@surma
Copy link
Collaborator Author

surma commented Feb 25, 2019

This is ready for final review and hopefully a merge :D

I pushed the small batch processing demo to a glitch, using the real SDK file exposed from this PR endpoint.

@jakearchibald PTAL

@surma surma changed the title WIP: Client-side API Client-side API Feb 25, 2019
package.json Outdated
@@ -4,9 +4,10 @@
"version": "1.3.3",
"license": "apache-2.0",
"scripts": {
"build:sdk": "microbundle --compress -f es -o build -i src/sdk.ts",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think --compress is the default and -i is optional.

Suggested change
"build:sdk": "microbundle --compress -f es -o build -i src/sdk.ts",
"build:sdk": "microbundle -f es -o build src/sdk.ts",

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be adding a load of definition files into the build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Do you think that’s a problem? There’s technically no harm in emitting them and I have no easy way to teach microbundle to not do that.

@surma
Copy link
Collaborator Author

surma commented Mar 5, 2019

@jakearchibald ping :)

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this create a privacy problem?

Eg, evil.com contains a link "Check out squoosh.app!" which, when clicked, opens a new window. To the user it looks like a regular tab with "squoosh.app" in the address bar, but if they drop any private images onto the site, evil.com can read it.

It seems like we need to limit usage of this API to cases where the source of control is obvious, eg iframes, as the URL bar will display evil.com rather than squoosh.app.

# Module imports need CORS, so we enable it for the SDK file.
/squoosh.mjs
Access-Control-Allow-Origin: *

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the header to all responses. May as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resource has a max-age of 31536000. Feels like it should be no-cache right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, what's the benefit of self-hosting this rather than providing it as a separate package? I'm worried about making changes to this resource that breaks people's sites.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that this way at least SDK and Squoosh stay in sync in terms of versioning. We can also not host it and publish it as a package, but not sure it’s that different in terms of breakage.

package.json Outdated
@@ -4,9 +4,10 @@
"version": "1.3.3",
"license": "apache-2.0",
"scripts": {
"build:sdk": "microbundle --compress -f es -o build -i src/sdk.ts",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be adding a load of definition files into the build.

}`,
);
}
ev.stopPropagation();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we already discussed this, but what's the benefit here? What other kind of listeners do we expect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was that there could be a scenario where an squoosh iframe is reused and the next user might send a new READY? message. It’s mostly me playing it safe.

src/components/compress/index.tsx Show resolved Hide resolved
@surma
Copy link
Collaborator Author

surma commented Mar 20, 2019

Hopefully this limits it to iframes only.

@mbrevda
Copy link

mbrevda commented Jun 18, 2019

Is this blocked on something?

@vroussel35
Copy link

Hi guys,
Can you let us know when you plan to deliver this request?
(no pressure but having a little bit of visibility on this client-side API functionality would be great to plan on our side)
Looking forward to hear from you.
Thanks

@usualoma
Copy link

Great PR!
This PR only works on Google Chrome or Firefox, but it is still useful enough.

This is an example of integration with a CMS created based on this PR.
https://github.com/usualoma/mt-plugin-Squoosh/

I hope this PR will be merged into master.

@surma surma closed this Jun 15, 2020
@mbrevda
Copy link

mbrevda commented Jun 15, 2020

Any context on why this was closed?

@dgrammatiko
Copy link

Any context on why this was closed?

Pretty sure this was due to moving the branch master to live and in order to remove the first one this was necessary. I've faced that on my own repos. I'll guess @surma will reopen it again at some point

@mbrevda
Copy link

mbrevda commented Jun 15, 2020

Pretty sure you can just select a new branch for the PR without needing to close it

@developit
Copy link
Collaborator

Changing the master branch on GitHub auto-closes all PR's targeted at that branch.

@surma surma reopened this Jun 16, 2020
@surma surma changed the base branch from master to dev June 16, 2020 10:59
@surma
Copy link
Collaborator Author

surma commented Dec 10, 2020

Closing this for now in favor of the CLI for batch processing (and also Share Target fulfills most of the purpose).

@surma surma closed this Dec 10, 2020
@mbrevda
Copy link

mbrevda commented Dec 10, 2020

Can Share Target be used to programmatically compress images client side (without user interaction)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants