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

Reduce @squoosh/lib Node.js API usage (make it run on the web) #1123

Merged
merged 10 commits into from
Sep 8, 2021

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Aug 16, 2021

  • Remove fs since its Node.js only
  • Remove os since its Node.js only
  • Remove other types of inputs besides ArrayBuffer

This lays the foundation for making @squoosh/lib run on the web.

Related to #1084

@styfle styfle changed the title Add loadFile parameter to ImagePool constructor Reduce Node.js API surface for @squoosh/lib (make it run on the web) Aug 20, 2021
@styfle styfle changed the title Reduce Node.js API surface for @squoosh/lib (make it run on the web) Reduce Node.js API usage in @squoosh/lib (make it run on the web) Aug 20, 2021
@styfle styfle changed the title Reduce Node.js API usage in @squoosh/lib (make it run on the web) Reduce @squoosh/lib Node.js API usage (make it run on the web) Aug 20, 2021
Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @styfle (and apologies for the delay).

I want to give both @ergunsh and @atjn a chance to weigh in on this, but then we can go ahead and get this merged (and I should probably whip up some sort of release notes as this is a pretty major breaking change).

Copy link
Contributor

@atjn atjn left a comment

Choose a reason for hiding this comment

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

If the plan is to make this code fully platform-agnostic, then it looks great, I just found some minor issues :)

However, I believe we agreed that it would be nice to have wrappers for specific platforms, which would (among other things) still allow passing file paths to ingestImage, and still allow the default worker_threads to be set with cpus.length?
If that is still the plan, I would suggest making that wrapper before publishing a new version. These changes are very breaking, and it is pretty annoying for all parties involved to be forced to implement these new changes, only to see them be reverted in the near future.

libsquoosh/README.md Outdated Show resolved Hide resolved
libsquoosh/README.md Outdated Show resolved Hide resolved
libsquoosh/README.md Outdated Show resolved Hide resolved
Co-authored-by: Anton <dev@atjn.dk>
@google-cla
Copy link

google-cla bot commented Sep 2, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed CLA: Yes labels Sep 2, 2021
@google-cla
Copy link

google-cla bot commented Sep 2, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@styfle
Copy link
Contributor Author

styfle commented Sep 2, 2021

it would be nice to have wrappers for specific platforms

I don't think it's worth the maintenance burden because the consumer only needs to add one line of code to get CPU count.

If we wanted to do this, we would need to make sure CI tested each of those platforms (it looks like there isn't even any tests yet at all 😬 )

Let's keep @squoosh/lib platform agnostic.

@styfle
Copy link
Contributor Author

styfle commented Sep 2, 2021

I just realized, the only remaining Node.js API is worker_threads.

Should I swap that out for web-worker in this PR or create a new PR after this one is merged?

@surma
Copy link
Collaborator

surma commented Sep 3, 2021

Hm, I’m torn. I kind agree with @atjn that this is pretty breaking and it’d be nice to provide wrappers. That being said, we have removed paths as parameters from ingestImage(), so to avoid a breaking change the wrapper would have to wrap that call as well. In the end, if there’s a time to do breaking changes it’s now. We are still in v0 territory and @squoosh/lib is still in its early days of adoption. I’m leaning towards making the breaking change here and publishing a CHANGELOG.md when we make the next release.

To your question, @styfle: Yes, let’s pull in web-worker. If there are problems with that, we should work with @developit to fix them.

@styfle
Copy link
Contributor Author

styfle commented Sep 3, 2021

@surma @developit I tried to convert from Node.js worker_threads to the web-worker package but I think this will require a bigger refactor. In particular, I don't think there's a way to check for isMainThread or parentPort.

Any thoughts on how to restructure this code to make web-worker fit here?

if (!isMainThread) {
WorkerPool.useThisThreadAsWorker(handleJob);
}

libsquoosh/README.md Outdated Show resolved Hide resolved
@developit
Copy link
Collaborator

I'm doing some work locally based on this PR to try to get @squoosh/lib working in both Node + Browser. Thanks for the awesome work @styfle - here's hoping the WASM loading ends up being possible 🤞

@styfle
Copy link
Contributor Author

styfle commented Sep 8, 2021

I'm doing some work locally based on this PR to try to get @squoosh/lib working in both Node + Browser.

@developit Thats great to hear!

Are you going to submit it as a new PR with all the changes or should we merge this PR first and you create a follow up?

@surma
Copy link
Collaborator

surma commented Sep 8, 2021

@styfle Good Q. I’d say lets merge this (do you mind doing a re-base to make sure everything is still working with @ergunsh’s work?). And then Jason and I can follow up on the worker/wasm issue separately.

@google-cla
Copy link

google-cla bot commented Sep 8, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@styfle
Copy link
Contributor Author

styfle commented Sep 8, 2021

@surma Everything is up to date and the CLI works as expected 👍

Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

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

Thanks so much, @styfle!

@surma surma merged commit 64cad1c into GoogleChromeLabs:dev Sep 8, 2021
@styfle styfle deleted the load-file-async branch September 8, 2021 21:32
@developit
Copy link
Collaborator

developit commented Sep 9, 2021

Thanks @styfle

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

Successfully merging this pull request may close these issues.

5 participants