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

Make optional props actually optional #203

Merged
merged 2 commits into from
Jun 24, 2022
Merged

Make optional props actually optional #203

merged 2 commits into from
Jun 24, 2022

Conversation

zpao
Copy link
Owner

@zpao zpao commented Jun 18, 2022

This drops use of defaultProps in favor of inline destructuring with default assignment.

This should effectively be type compatible with the rest of 3.x (slightly different but close enough) and it prepares for an easier implementation of forwardRef (which will be in 4.0 since I don't want to support that and the default QRCode export)

Fixes #197

Comment on lines +190 to +194
// We're just using this state to trigger rerenders when images load. We
// Don't actually read the value anywhere. A smarter use of useEffect would
// depend on this value.
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const [isImgLoaded, setIsImageLoaded] = useState(false);
Copy link
Owner Author

Choose a reason for hiding this comment

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

This would get better in a future version. I wanted behavior to remain the same in 3.x (always update) but in 4.x we'd use this as an input to useEffect

zpao added 2 commits June 24, 2022 11:44
This drops the use of React's defaultProps in favor of destructuring,
which results in some cascading changes. This should ultimately also
make forwardRef easier to implement in the future without getting too
fancy.
@zpao
Copy link
Owner Author

zpao commented Jun 24, 2022

For posterity, this is the diff of the type declarations. You can see that there are only 2 notable differences:

  1. Removal of the additional namespace for each component, which housed the defaultProps.
  2. Some props being made optional.

While technically we could argue that the removal of the namespace is a breaking change and cause for a major version, I don't agree.

--- index.d.ts	2022-06-24 12:01:29.000000000 -0700
+++ lib/index.d.ts	2022-06-24 12:00:46.000000000 -0700
@@ -1,65 +1,38 @@
 import React, { CSSProperties } from 'react';
 
 /**
  * @license qrcode.react
  * Copyright (c) Paul O'Shannessy
  * SPDX-License-Identifier: ISC
  */
 
+declare type ImageSettings = {
+    src: string;
+    height: number;
+    width: number;
+    excavate: boolean;
+    x?: number;
+    y?: number;
+};
 declare type QRProps = {
     value: string;
-    size: number;
-    level: string;
-    bgColor: string;
-    fgColor: string;
+    size?: number;
+    level?: string;
+    bgColor?: string;
+    fgColor?: string;
     style?: CSSProperties;
-    includeMargin: boolean;
-    imageSettings?: {
-        src: string;
-        height: number;
-        width: number;
-        excavate: boolean;
-        x?: number;
-        y?: number;
-    };
+    includeMargin?: boolean;
+    imageSettings?: ImageSettings;
 };
 declare type QRPropsCanvas = QRProps & React.CanvasHTMLAttributes<HTMLCanvasElement>;
 declare type QRPropsSVG = QRProps & React.SVGProps<SVGSVGElement>;
 declare function QRCodeCanvas(props: QRPropsCanvas): JSX.Element;
-declare namespace QRCodeCanvas {
-    var defaultProps: {
-        size: number;
-        level: string;
-        bgColor: string;
-        fgColor: string;
-        includeMargin: boolean;
-    };
-}
 declare function QRCodeSVG(props: QRPropsSVG): JSX.Element;
-declare namespace QRCodeSVG {
-    var defaultProps: {
-        size: number;
-        level: string;
-        bgColor: string;
-        fgColor: string;
-        includeMargin: boolean;
-    };
-}
 declare type RootProps = (QRPropsSVG & {
     renderAs: 'svg';
 }) | (QRPropsCanvas & {
-    renderAs: 'canvas';
+    renderAs?: 'canvas';
 });
-declare const QRCode: {
-    (props: RootProps): JSX.Element;
-    defaultProps: {
-        size: number;
-        level: string;
-        bgColor: string;
-        fgColor: string;
-        includeMargin: boolean;
-        renderAs: string;
-    };
-};
+declare const QRCode: (props: RootProps) => JSX.Element;
 
 export { QRCodeCanvas, QRCodeSVG, QRCode as default };

@zpao zpao added this to the 3.1.0 milestone Jun 24, 2022
@zpao zpao merged commit c956d5c into trunk Jun 24, 2022
@zpao zpao deleted the optional-props branch June 25, 2022 22:06
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.

Make QRProps optionals
1 participant