Add disable_scripts non-global option

[osbre]

This option got removed in #168, my PR brings it back and makes it possible to use both ways.

I personally have a use case for disabling scripts not globally, but for particular pages. Puppeteer and others also have this option. Would love to see it in this (best-in-class) Elixir library

Closes #267

[MaeIsBad]

Looks fine by me, but I would add a note in the docs of the global disable_scripts option to mention that it is only the default, and that it can be overwritten by the local one

[andreasknoepfle]

Hey @osbre,

besides the added note in the docs @MaeIsBad already suggested, could you add tests for setting up both global and local options at the same time, so that we have cases where the local option will override the global one.

I think then it is good to go for my point of view.
Let me know if you need any help with this.

Cheers
Andi

[osbre]

Thanks, @andreasknoepfle and @MaeIsBad, I've added the requested changes.

[andreasknoepfle]

Looks. good to me. One mix format and I can merge :)

[osbre]

@andreasknoepfle the "Lint" workflow fails with the message that one file wasn't formatted. I'm thinking - is it because I used the latest Elixir version to run mix format? Perhaps updating .tool-versions can fix this issue?

[maltoe]

@osbre The lint workflow is currently run on our debian-bullseye image which contains Elixir 1.14 (see here and here). Proper fix IMO would be to instead run this workflow on a stock ubuntu-latest github actions profile with the "normal" setup-elixir way of installing Elixir (ideally adhering to the .tool-versions file as you mention), as it doesn't require Chrome or ghostscript to execute. Quick fix would be for you to install Elixir as set in.tool-versions and run mix format there. Alternatively just manually revert the offending formatting changes.

[edit] hijacked #214 to track the proper solution

[maltoe]

Hi @osbre

Sorry for arriving so late to bring up the following issues. Was away for a few weeks and only yesterday thought about it a bit more.

1. With the way how the global options are merged into the job args here, the conditional in Navigate will be executed on every run, even if only the global option is given. Which may be ok, but perhaps the code in SpawnSession should be removed then.
2. We must re-enable scripts in ResetTarget after use of this option, as otherwise the disabled scripts will leak into subsequent runs.

---

That said, I also wonder the PR in general, as it breaks the current global vs. non-global option setup. The original idea was to perform as much work as possible in the session initialization, aiming at lower "redundant" work per print job and hence improving performance. Not sure whether the performance benefit is measureable at all, but at least that has been the basic reasoning behind which options are global and which are per-job and this PR puts an end to this "practice", yet only for one of the relevant options - offline, set_user_agent, ignore_certificate_errors would be other candidates for the same, which I'm sure would follow suit at some point.

At the same time, users already can have spawn multiple (named) ChromicPDF supervisor instances. Which allows them to, for example, have separate MyApp.ChromicPDF.Default and MyApp.ChromicPDF.NoScripts interfaces.

This makes me wonder, whether we should rather invest in this area, e.g. by improving documentation of ChromicPDF.Supervisor to point users in this direction?

[maltoe]

@osbre Had an idea and implemented a poc: #273 -> Would this solve your use-case?

[maltoe]

@osbre thanks again for your work 💜

After some thought and more work on #273 I believe it is the better approach to let the options be either "session options" (run at initialization, so only once) or "pdf options", evaluated per print job.