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

bug: missing export of DatetimeHighlightStyle #27353

Closed
3 tasks done
aparajita opened this issue May 2, 2023 · 5 comments · Fixed by #27360
Closed
3 tasks done

bug: missing export of DatetimeHighlightStyle #27353

aparajita opened this issue May 2, 2023 · 5 comments · Fixed by #27360
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@aparajita
Copy link

aparajita commented May 2, 2023

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

Although the source does export DatetimeHighlightStyle, the built package does not, which results in a TS error when trying to import it.

Expected Behavior

DatetimeHighlightStyle should be exported in the built package.

Steps to Reproduce

  1. Open any project.
  2. Enter the following:
import { DatetimeHighlightStyle } from '@ionic/core'
  1. TypeScript will complain that @ionic/core does not export that name.

Code Reproduction URL

https://stackblitz.com/edit/xkg3uz?file=src/main.ts

Ionic Info

Ionic:

   Ionic CLI : 7.1.1 (/Users/aparajita/Developer/projects/clients/willsub/apps-monorepo/node_modules/.pnpm/@ionic+cli@7.1.1/node_modules/@ionic/cli)

Capacitor:

   Capacitor CLI      : 4.8.0
   @capacitor/android : 4.8.0 (/Users/aparajita/Developer/projects/clients/willsub/apps-monorepo/node_modules/.pnpm/@capacitor+android@4.8.0_@capacitor+core@4.8.0/node_modules/@capacitor/android)
   @capacitor/core    : 4.8.0 (/Users/aparajita/Developer/projects/clients/willsub/apps-monorepo/node_modules/.pnpm/@capacitor+core@4.8.0/node_modules/@capacitor/core)
   @capacitor/ios     : 4.8.0 (/Users/aparajita/Developer/projects/clients/willsub/apps-monorepo/node_modules/.pnpm/@capacitor+ios@4.8.0_@capacitor+core@4.8.0/node_modules/@capacitor/ios)

Utility:

   cordova-res : not installed globally
   native-run  : not installed globally

System:

   NodeJS : v20.0.0 (/Users/aparajita/Library/pnpm/nodejs/20.0.0/bin/node)
   npm    : 9.6.4
   OS     : macOS Unknown

Additional Information

The relevant line in the built package is @ionic/core/types/components.d.ts, lines 18 and 54, which sure enough are missing the relevant name:

import { DatetimeChangeEventDetail, DatetimeHighlight, DatetimeHighlightCallback, DatetimePresentation, TitleSelectedDatesFormatter } from "./components/datetime/datetime-interface";

export { DatetimeChangeEventDetail, DatetimeHighlight, DatetimeHighlightCallback, DatetimePresentation, TitleSelectedDatesFormatter } from "./components/datetime/datetime-interface";
@ionitron-bot ionitron-bot bot added the triage label May 2, 2023
@sean-perkins sean-perkins added package: core @ionic/core package type: bug a confirmed bug report labels May 3, 2023
@ionitron-bot ionitron-bot bot removed the triage label May 3, 2023
@sean-perkins
Copy link
Contributor

Hello @aparajita thanks for this issue! Can you share your intended usage requiring the DatetimeHighlightStyle type?

To bind to highlightedDates, consumers should need DatetimeHighlight or DatetimeHighlightCallback. Are you requiring the type for the return type of DatetimeHighlightCallback?

@sean-perkins sean-perkins self-assigned this May 3, 2023
@aparajita
Copy link
Author

Hey @sean-perkins, first of all, since DatetimeHighlight is defined as:

export type DatetimeHighlight = {
  date: string;
} & DatetimeHighlightStyle;

not exporting DatetimeHighlightStyle does not allow us to set the style separately from the date in a type-safe manner. I'm not quite sure what the rationale is behind that, if indeed it was a conscious choice.

Here's the relevant portions of my code:

  const kDateHighlights: Record<string, DatetimeHighlightStyle> = {
    available: {
      textColor: 'var(--ion-color-dark-contrast)',
      backgroundColor: 'var(--color-available-job)',
    },
    assigned: {
      textColor: 'var(--ion-color-dark-contrast)',
      backgroundColor: 'var(--color-assigned-job)',
    },
    pto: {
      textColor: 'var(--ion-color-dark-contrast)',
      backgroundColor: 'var(--color-pto)',
    },
  }

  function getDateHighlight(item: ListItem): DatetimeHighlightStyle {
    if (isAvailableJob(item)) {
      return kDateHighlights.available
    } else if (isAssignedJob(item)) {
      return kDateHighlights.assigned
    } else {
      return kDateHighlights.pto
    }
  }

  // Not exactly my code, but the general idea
  const highlightedDates = computed((): DatetimeHighlight[] => {
    const highlights: DatetimeHighlight[] = []

    for (const item of items) {
      // Without DatetimeHighlightStyle being defined, TS sees the object
      // parameter as `any`.
      highlights.push({
        date: item.date,
        ...getDateHighlight(item)
      })
    }
  })

@sean-perkins
Copy link
Contributor

@aparajita thanks for the code snippet, that makes sense!

Stencil v3 includes a feature that automatically exports types assigned to a @Prop. Prior to this, Ionic Framework had to manually export all individual types. The downside to this, is that types that are consumed in those automatically exported types, are not additionally exported. That is the case here, where we missed exporting this lower-level type for developers.

I will open #27360 for review.

@aparajita
Copy link
Author

Thanks @sean-perkins. I am able to work around it for now by declaring the type myself.

@sean-perkins sean-perkins removed their assignment May 3, 2023
sean-perkins added a commit that referenced this issue May 4, 2023
Issue number: Resolves #27353

---------

<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

`DatetimeHighlightStyle` is not automatically exported from
`@ionic/core`, since the type is not directly referenced on a prop.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Manually exports `DatetimeHighlightStyle` to consumers

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
@ionitron-bot
Copy link

ionitron-bot bot commented Jun 3, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants