Make optional peer dependencies actually optional

[octogonz]

# For TypeScript Users
# (see "I use Typescript and get TS2307: Cannot find module errors" section below)
npm i -D @types/node rrule moment-timezone moment dayjs @types/luxon

For TypeScript users, the above advice defeats the purpose of declaring the vendor packages as "optional." This is unfortunate, since people who use TypeScript tend to be the audience who cares most about rigorous management of dependencies.😉

Is there a better solution? Here's some ideas:

Idea 1: "Opt-in" by decoupling the API

The ideal would be to redesign the API so that the vendor integrations are opt-in via specific imports, rather than being tightly coupled. (This is the same idea behind Day.js plugins.)

For example, instead of this:

src/calendar.ts

. . .
import type {Duration} from 'moment-timezone';
. . .
export interface ICalCalendarData {
    ttl?: number | Duration | null;
}

We might do something like this:

src/calendar.ts

. . .
export type ICalVendorDuration = { __type: 'ICalVendorDuration' };;
. . .
export interface ICalCalendarData {
    ttl?: number | ICalVendorDuration | null;
}

If someone has moment, one approach would be to type-check by importing an adapter:

example-consumer.ts

import type {Duration} from 'moment-timezone';
import ical from 'ical-generator';
import icalMoment from 'ical-generator/moment';

function example(duration: Duration): void {
  ical({
    ttl: icalMoment(duration)
  });
}

This is just one possibility. There are many ways to design an API that cleanly separates its dependencies. Doing so might also eliminate the awkward Webpack workaround that arose because one small feature entangles the entire bundle with Node.js fs.

Idea 2: Replace the dependencies with stub types

The ical-generator API is very disciplined about accepting vendor objects as inputs, but never exposing them as outputs. Thus, the type declarations don't actually provide operations, they merely check for an incorrect input. Since TypeScript uses structural typing instead of nominal typing, the ical-generator package could provide its own types:

// Instead of using this type...
import type {Duration} from 'moment-timezone';

// ...the ical-generator's API could export and use this substitute:

/**
 * This is a stub for the `Duration` interface from the `moment` NPM package.
 * The stub doesn't include the full set of properties, just enough of the type
 * signature to conflict with an incorrect input.
 */
export interface MomentDuration {
  clone(): any;
  locale(): string;
  toISOString(): string;
  abs(): any;
  isValid(): boolean;
}

(Compare with the real moment/ts3.1-typings/moment.d.ts.)

Idea 3: "Opt-out" via a stub import

A final possibility would be to provide a stub that consumers can import if they do want type checking for ical-generator but don't care about any vendor integrations.

ical-generator/dist/disable-integrations.d.ts

declare module 'moment-timezone' {
  export interface Moment { __type: 'disabled' }
  export interface Duration { __type: 'disabled' }
}

declare module 'rrule' {
  export interface RRule { __type: 'disabled' }
}

declare module 'moment' {
  export interface MomentFormatSpecification { __type: 'disabled' }
  export interface Moment { __type: 'disabled' }
  export interface Duration { __type: 'disabled' }
}

declare module 'dayjs' {
  export interface Dayjs { __type: 'disabled' }
}

declare module 'luxon' {
  export interface DateTime { __type: 'disabled' }
}

Then the consumer can avoid installing irrelevant peer dependencies like this:

example-consumer.ts

/// <reference path="../node_modules/ical-generator/dist/disable-integrations.d.ts" />
import ical from 'ical-generator';

Idea 3 is how I worked around the problem in my own project, but it seems like the most clumsy of these ideas.

[sebbo2002]

Hi @octogonz, I'm still on vacation until July 17, so please excuse my very short answer.

Solution number 1 has been on my todo list for quite some time for the next refactoring with several breaking changes, which I haven't gotten to yet due to time constraints.

I haven't thought about solution number 2 at all and it seems very interesting to me because it would be implementable with relatively manageable effort and with few (if not completely no) breaking changes. I have to look at that after the vacation in again, thank you for this idea.

And about No. 3 I think we do not really need to talk big I think 😂

[octogonz]

Thanks for the reply. Have a good vacation!

[sebbo2002]

🎉 This issue has been resolved in version 3.5.0-develop.1 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release
• v3.5.0-develop.1

Your semantic-release bot 📦🚀

[sebbo2002]

Hi @octogonz, sorry it took so long now. I have implemented suggestion no. 2 and published it in version 3.5.0-develop.1. Feel free to have a look if this fits for you now, am open for feedback.

[octogonz]

Hi @octogonz, sorry it took so long now. I have implemented suggestion no. 2 and published it in version 3.5.0-develop.1. Feel free to have a look if this fits for you now, am open for feedback.

I tested [email protected] and it solved my problem. 👍👍

Thank you for making this fix!

[sebbo2002]

🎉 This issue has been resolved in version 3.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release
• v3.5.0

Your semantic-release bot 📦🚀