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

TypeScript: WizardTaskItem and CollapsibleBody require a className prop (should be optional) #9142

Closed
haszari opened this issue Jul 22, 2024 · 5 comments · Fixed by #9157
Closed
Assignees
Labels
focus: tax (VAT) documents good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. type: task The issue is an internally driven task (e.g. from another A8c team). type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@haszari
Copy link
Contributor

haszari commented Jul 22, 2024

Describe the bug

No merchant impact, this bugged me when working on #8926, so logging an issue.

  • In the tax info modal, various components have classNames={null} which seems redundant and unnecessary.
  • I tried removing, to see what breaks.
    • No functionality or UI breaks.
    • There is a TypeScript error:
      • TS2741: Property 'className' is missing in type '{ children: (false | "" | Element | null)[]; }' but required in type '{ [x: string]: any; className: any; }'.

I think this can be fixed by adding a default (e.g. classNames = null) to the relevant JS components (WizardTaskItem, CollapsibleBody).

Curious for insights from TypeScript experts @Jinksi @reykjalin .

@haszari haszari added type: bug The issue is a confirmed bug. type: task The issue is an internally driven task (e.g. from another A8c team). focus: tax (VAT) documents and removed type: bug The issue is a confirmed bug. labels Jul 22, 2024
@reykjalin
Copy link
Contributor

@haszari do you have a PR I can look at? The problem should be fixable by making the classNames prop optional on the type. Instead of a type like

interface WizardTaskItemProps {
    // some other props…
    classNames: string;
}

Make the classNames optional:

interface WizardTaskItemProps {
    // some other props…
    classNames?: string; // Note the `?` to make the prop optional.
}

If the components are JS components it should be possible to define the types using JSDoc comments in the component files themselves:

/**
 * @param {string} [classNames] The classes assigned to the task item.
 */
export const WizardTaskItem = ( { /* other props */, classNames } ) => {
    // …
};

This will all mean undefined (i.e. not providing a prop) is possible. If we explicitly want null to be an option that must be specified:

interface WizardTaskItemProps {
    // some other props…
    classNames: string | null; // Must provide either a string or null, not providing the prop is not an option.
}

interface WizardTaskItemProps {
    // some other props…
    classNames?: string | null; // string, null, or undefined allowed, not providing the prop **is an option**.
}
/**
 * @param {string|null} classNames Must provide either a string or null, not providing the prop is not an option.
 */
export const WizardTaskItem = ( { /* other props */, classNames } ) => {
    // …
};

/**
 * @param {string|null} [classNames] string, null, or undefined allowed, not providing the prop **is an option**.
 */
export const WizardTaskItem = ( { /* other props */, classNames } ) => {
    // …
};

@reykjalin
Copy link
Contributor

The JSDoc syntax supported by TS is documented here.

@haszari
Copy link
Contributor Author

haszari commented Jul 22, 2024

do you have a PR I can look at?

That's the PR where I encountered it, but I haven't deleted the classNames = {null} on that PR branch. I can spin up a PR demonstrating the bug, basically delete classNames={null} in a couple of places!

I think this can be fixed by adding a default (e.g. classNames = null) to the relevant JS components (WizardTaskItem, CollapsibleBody).
The problem should be fixable by making the classNames prop optional on the type.

For sure, that's what I was assuming. Adding a default param (to the JavaScript definition) fixed the bug, and seems sensible to me. @reykjalin what do you think of that fix – is it "hacky" in some way?

It seems an optional bonus to define these components as TypeScript types, I'd default to leaving them implicit, but I'm curious to hear rationale for why making the types explicit would be better.

@reykjalin
Copy link
Contributor

Adding a default param (to the JavaScript definition) fixed the bug, and seems sensible to me

Ah yes, that's even cleaner! I just didn't realize that was an option since it seemed based on the issue description that it should be possible to pass null to indicate some form of "nothingness", as opposed to using a default value.

So long as using a default value matches the previous version of the API I see no problem with doing that 👍

@reykjalin
Copy link
Contributor

To elaborate a bit, here's an example where it wouldn't match:

// Previous. Silly example
export const WeirdComponent = ( { classNames } ) => (
  <div className={ classNames }>
  </div>
);

// New. Still silly example.
export const WeirdComponent = ( { classNames = 'default' } ) => (
  <div className={ classNames }>
  </div>
);

The difference between the 2 is that the previous version allows null and undefined whereas the new one does not. That would be a change I wouldn't recommend. But then you can just make the default value undefined or null, in which case the change is fine.

All of this to say: make sure the default value matches the previous implementation and you should be fine! 🙂

@haszari haszari self-assigned this Jul 24, 2024
@haszari haszari added good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. type: technical debt This issue/PR represents/solves the technical debt of the project. labels Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: tax (VAT) documents good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. type: task The issue is an internally driven task (e.g. from another A8c team). type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
2 participants