-
-
Notifications
You must be signed in to change notification settings - Fork 7
Fix/fetch printers #27
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
Conversation
|
Changes to the upgrade guide can be found here: |
|
Couldn't we just make the parameters optional for now? |
|
We could. They would always be null, so peoples code would not error. However if they rely on those properties their code would likely stop to work as intended. If it was me I’d rather see an error. Or alternatively a obvious warning. |
|
I mean so that it's not breaking now. We could deprecate, and then pull them out in the next major version? |
|
We could do that, but pardon my confusion. Wouldn't a deprecation mean the feature should keep working? Because if I set these values to null now, it would not mean the userland code keeps working like before. |
Maybe I'm missing something... from the linked issues it sounds like this isn't working right now? I'm just thinking that removing these would almost certainly cause a BC break for folks relying on them. But maybe this is only an internal data object and isn't used publicly? Or maybe its usage is minimal (hard to know)? Just thinking what's the path that lets us avoid a breaking change? |
It's def a publicly used class
I might have a solution based on what @PeteBishwhip suggested 👍🏻 Let me try that right now |
Fixes #20
Fixes #21
Turns out Electron removed some properties we missed in the update:
https://www.electronjs.org/docs/latest/breaking-changes#removed-isdefault-and-status-properties-on-printerinfo
I've removed these properties from the Printer value object. This might be breaking though. Should I retroactively add it to the upgrade guide @simonhamp ?