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

fix: bad error passing webContents.print(null) #38612

Merged
merged 1 commit into from Jun 7, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 6, 2023

Description of Change

Fixes #38598.

Fixes an issue where passing webContents.print(null) would trigger the following error:

Cannot read properties of null (reading 'pageSize').

This was happening because we weren't properly validating options.

Checklist

Release Notes

Notes: Fixes an issue where passing webContents.print(null) could incorrectly trigger an error.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jun 6, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 6, 2023
Comment on lines +340 to +341
WebContents.prototype.print = function (printOptions: ElectronInternal.WebContentsPrintOptions, callback) {
const options = printOptions ?? {};
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker to the PR; asking for my own info: what's the difference between the default = {} argument in the old code and the options = printOptions ?? {} code? Is this to safeguard against an explicit null being passed as the first argument, or...?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ckerr default arguments only take effect if the no value or undefined is passed, so when they user passes in null they're bypassed entirely . This change (and using ?? specifically) ensures it's defaulted if it's null or undefined.

Old but nevertheless interesting article about falsy value handling patterns: https://medium.com/@bmb21/default-parameters-and-nulls-in-javascript-9149c8d26b0a

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 7, 2023
@codebytere codebytere merged commit c5972ba into main Jun 7, 2023
18 checks passed
@codebytere codebytere deleted the fix-printing-null-options branch June 7, 2023 17:18
@release-clerk
Copy link

release-clerk bot commented Jun 7, 2023

Release Notes Persisted

Fixes an issue where passing webContents.print(null) could incorrectly trigger an error.

@trop
Copy link
Contributor

trop bot commented Jun 7, 2023

I have automatically backported this PR to "24-x-y", please check out #38640

@trop trop bot added in-flight/24-x-y and removed target/24-x-y PR should also be added to the "24-x-y" branch. labels Jun 7, 2023
@trop
Copy link
Contributor

trop bot commented Jun 7, 2023

I have automatically backported this PR to "26-x-y", please check out #38641

@trop
Copy link
Contributor

trop bot commented Jun 7, 2023

I have automatically backported this PR to "25-x-y", please check out #38642

@trop trop bot added in-flight/26-x-y in-flight/25-x-y merged/25-x-y PR was merged to the "25-x-y" branch. and removed target/26-x-y PR should also be added to the "26-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. in-flight/25-x-y labels Jun 7, 2023
@trop trop bot added merged/26-x-y PR was merged to the "26-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch and removed in-flight/26-x-y in-flight/24-x-y labels Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: webContents.print fails with unspecified options
2 participants