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

Add ability to remove default args via config #314

Merged

Conversation

walter
Copy link
Contributor

@walter walter commented Jul 23, 2024

When trying to use chromic_pdf in combination with pages that contain WebGL images, there are two default args in ChromeRunner that prevent it from working.

  • --headless (rather than --headless=new)
  • --disable-gpu which, despite its name, will disable software based webgl rendering in addition to hardware accelerated webgl

Based on my experimentation, simply overriding these in :chrome_args is not enough. Once they are declared, they can't be overridden by further switches.

Instead of just taking these out, and maybe break other people's projects, I think adding the ability to remove the args via configuration is a more useful feature.

Copy link
Collaborator

@maltoe maltoe left a comment

Choose a reason for hiding this comment

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

Hi @walter,

thanks for the contribution! 💜 Code looks good to me, though I'd prefer if we could extend chrome_args instead of adding a new option for this.

Please add yourself to the top of the changelog as well, something like

## Unreleased

### Added

- ... (@walter)

lib/chromic_pdf.ex Outdated Show resolved Hide resolved
@walter walter requested a review from maltoe July 23, 2024 23:08
Copy link
Collaborator

@maltoe maltoe left a comment

Choose a reason for hiding this comment

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

Hi @walter

Had a few stylistic nitpicks still. If you don't want to continue working on this, I can also merge it in and refactor afterwards. As you prefer.

Thanks again!

remove: ["--headless", "--disable-gpu"]]]
end

_Note: `:append` expects a string whereas `:remove` expects a list._
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be liberal in what the API can accept; a List.wrap/1 somewhere will allow you to accept strings or lists for either of the options, and then we don't need this note in the docs.

Suggested change
_Note: `:append` expects a string whereas `:remove` expects a list._

@@ -226,7 +226,7 @@ defmodule ChromicPDF.Supervisor do
@type local_chrome_option ::
{:no_sandbox, boolean()}
| {:discard_stderr, boolean()}
| {:chrome_args, binary()}
| {:chrome_args, binary() | map()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

# above
@type extended_chrome_args :: [{:append, binary() | [binary()]} | {:remove, binary() | [binary()]}]
Suggested change
| {:chrome_args, binary() | map()}
| {:chrome_args, binary() | extended_chrome_args()}

|> append_if("--no-sandbox", no_sandbox?(opts))
|> append_if(to_string(opts[:chrome_args]), !!opts[:chrome_args])
|> append_chrome_args_if(opts[:chrome_args])
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 the implementation could use another round of polishing :)

Suggested change
|> append_chrome_args_if(opts[:chrome_args])
|> handle_chrome_args(opts[:chrome_args])

Maybe go for a unified handler function. 3 Clauses, nil if option not set, then a is_binary clause for the original behaviour, and a new is_list clause for handling the extended args. Extracting options from this list should then be something like:

remove = List.wrap(Keyword.get(extended, :remove, []))

@walter
Copy link
Contributor Author

walter commented Jul 24, 2024

Hi @walter

Had a few stylistic nitpicks still. If you don't want to continue working on this, I can also merge it in and refactor afterwards. As you prefer.

Thanks again!

Thanks for the feedback. Jibes with some thoughts I had myself, but was distracted by other work. I hadn't used List.wrap before, nice!

Have a look at latest and let me know if there is anything else.

@walter walter requested a review from maltoe July 24, 2024 22:24
@maltoe maltoe merged commit 8c21994 into bitcrowd:main Jul 25, 2024
1 of 6 checks passed
@maltoe
Copy link
Collaborator

maltoe commented Jul 25, 2024

Thanks again!

@maltoe
Copy link
Collaborator

maltoe commented Jul 25, 2024

Released as v1.16.1

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.

2 participants