v12 broke Typescript types

[spike-rabbit]

There were two undocumented breaking changes in v12:

With v11 something like this worked:

import Shepherd from 'shepherd.js';

const tour:  Shepard.Tour = new Shepherd.Tour();

This is broken in v12 as Shepard is no longer a namespace (it was before).
In v12, one has to write this as:

import Shepherd from 'shepherd.js';
import {Tour} from 'shepherd.js/tour'

const tour:  Tour = new Shepherd.Tour()

This is in my opinion a horrible DX, as Tour only acts as a type, as there is no tour.mjs file. So one cannot call new Tour().
On the other side, Shepherd.Tour cannot be used as a type, since (as already mentioned) Shepherd is not a namespace.

The second breaking change is, that moduleResolution: 'node' no longer works, as the types are part of the exports field, so one had to update the package.json to use moduleResolution: 'Bundler'.

It would be really cool, if you could improve the types to be at least compatible with v11.

[chuckcarpenter]

@spike-rabbit this is a set of changes to step towards embracing correct ESM publishing and ingestion. It is a major version bump because of these breaking changes. You can see more context on how we've landed here in this issue: #2785

We are open to suggestions and more specifically a PR if you think there's a more straightforward way to support all scenarios, we've just not discovered it.

If you're using moduleResolution: 'node' then you just have to use the /dist/cjs/tour import paths. So no configuration for https://github.com/arethetypeswrong/arethetypeswrong.github.io are actually broken, but do require changes.

[RobbieTheWagner]

@spike-rabbit this is only a problem if you want to explicitly set types like that, right? Like if you just call const Tour = new Shepherd.Tour() it knows the type of that.

[spike-rabbit]

I fully support moving to ESM, even if this requires a breaking change.
What I don't like is, that this change was nowhere (at least not easy to find) documented.

• there is no hint in the changelog
• the documentation also does not mention, that Shepherd is no longer a namespace

The other thing is, that the types does not match reality.
According to the types, this is valid (also suggested by IDEs):

import {Tour} from 'shepherd.js/tour'

const tour = new Tour();

But currently, this does work as this import would be resolved by a Bundler to: shepherd.js/dist/esm/tour.mjs. Unfortunately, this file does not exist.

I see a few potential fixes here:

• drop the 'shepherd.js/tour' types and bring back the namespace
• break-up shepherd.js into its parts, so that a tour.mjs exists
• fix the package json and also export everything from shepherd.mjs
  Instead of:

  {
    "./*": {
      "import": {
        "types": "./dist/esm/*.d.ts",
        "default": "./dist/esm/*.mjs"
      },
      "require": {
        "types": "./dist/cjs/*.d.cts",
        "default": "./dist/cjs/*.cjs"
      }
    }
  }

  do:

  {
    "./*": {
      "import": {
        "types": "./dist/esm/*.d.ts",
        "default": "./dist/esm/shepherd.mjs"
      },
      "require": {
        "types": "./dist/cjs/*.d.cts",
        "default": "./dist/cjs/shepherd.cjs"
      }
    }
  }

I am not familiar with rollup, so I cannot tell which approach would be best. As long as the type matches the actual code I am happy.

---

Yes, this still works: const Tour = new Shepherd.Tour()

[RobbieTheWagner]

@spike-rabbit I think what we want here is to ship just one TS file instead of separate ones. I am not sure how to accomplish that though. We will need some kind of util to roll them into one. Are there any you have had success with?

[RobbieTheWagner]

@spike-rabbit would you be able to pull down #2871 and build it and try it out? I am making it just build one declaration file now, but I am unsure if this is what you wanted or not or if it might break other things.

[chuckcarpenter]

fixed by #2871