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

Simplify dist to only include one TS file #2871

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Conversation

RobbieTheWagner
Copy link
Member

No description provided.

@spike-rabbit
Copy link

With this the exported types now matches what is actually exported in js.

There is just one thing, that is weird. Right now, there is no type exported for Tour and Step.
So to declare a type explicitly, one has to do this:

const tour: InstanceType<typeof Shepherd.Tour> = new Shepherd.Tour();

I guess the easiest solution would be to use the namespaces again in the shepherd.d.ts file (copied form v11):

import _Step from './step';
import _Tour from './tour';

declare namespace Shepherd {
  /**
   * The currently active tour instance
   */
  export const activeTour: _Tour | undefined;
  export import Step = _Step;
  export import Tour = _Tour;
}

export default Shepherd;

@chuckcarpenter
Copy link
Member

@spike-rabbit I guess I just don't understand why either not just allowing the implicit types to be fine or to do as you suggested const tour: InstanceType<typeof Shepherd.Tour> = new Shepherd.Tour(); for anything specific?

@chuckcarpenter
Copy link
Member

If you want to discuss more, perhaps either an example in codesandbox or join the discord could help? https://discord.gg/wdEBJ5Pt

@spike-rabbit
Copy link

thx for the invite. Technical wise, with this change everything is fine.

It is just for a developer not ideal. One sometimes need explicit types. In my case it is an explicit function return type, enforced by a linter.

The v11 approach was nicer, as Shepherd.Tour worked as a type and as class object. Using the InstanceType<...> is way less intuitive.

Anyway, I guess it is up to you whether you want to provide this. Apart from this, shepherd works pretty well for us, so thx for the nice library.

@RobbieTheWagner
Copy link
Member Author

@spike-rabbit we are at the mercy of tools that generate the declaration files for us. Because we use svelte, we cannot just run tsc and so this output was the best I have been able to get so far. We could import a bunch of these things into the shepherd.js file I suppose, just for the sake of reexporting them.

@RobbieTheWagner RobbieTheWagner marked this pull request as ready for review June 17, 2024 11:17
@RobbieTheWagner
Copy link
Member Author

@spike-rabbit I added more exports, can you please try this again?

@spike-rabbit
Copy link

I like that the Tour and others are now available as a type.

But the declaration is not ideal here. For instance tour is declared as a class:

declare class Tour extends Evented {
  ...
}

Which would allow consumers to do:

import {Tour} from 'shepherd.js';
const tour = new Tour();

but this does not work, as the mjs does not export Tour.

I see two options which I was also able to integrate in this PR:

  1. only export Tour and others as a real type only:
type TourType = Tour;
export type { TourType as Tour};

That way consumers would not be able to call new Tour().

  1. It seems like you are only using Shepherd.Tour to use an alternative implementation when running in a server context. You could move this into the Tour and Step constructor:
constructor() {
  if (typeof window === 'undefined') {
    return new TourNoOp();
  }

  ...
}

Then you could just export Tour and Step without type restrictions:

export * from './evented.ts';
export * from './step.ts';
export * from './tour.ts';

@RobbieTheWagner
Copy link
Member Author

@spike-rabbit we have tried several times to move the noop code around like you mention here. If you would like to open a PR, we would gladly accept it, but I have not been able to make it work.

If you move it like you suggested into Step's constructor, you get an error from TS Return type of constructor signature must be assignable to the instance type of the class.

only export Tour and others as a real type only:

Perhaps I am just not informed enough into how to make this work, but I don't think it will work since we are importing tour. Perhaps if we renamed it as you are suggesting, but I don't like it having a different name.

Copy link
Member

@chuckcarpenter chuckcarpenter left a comment

Choose a reason for hiding this comment

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

@chuckcarpenter chuckcarpenter merged commit 44e7b57 into main Jun 17, 2024
3 checks passed
@chuckcarpenter chuckcarpenter deleted the simplify-dist branch June 17, 2024 17:28
@github-actions github-actions bot mentioned this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants