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

Add Native Typescript Declarations #375

Merged
merged 10 commits into from
Jun 20, 2024
Merged

Add Native Typescript Declarations #375

merged 10 commits into from
Jun 20, 2024

Conversation

Kenneth-Sills
Copy link
Contributor

@Kenneth-Sills Kenneth-Sills commented Apr 9, 2024

Summary:

As requested in #15, this PR adds a very simple .d.ts file with Typescript declarations for both the Options and Components API.

The types were tested with:

  • The examples for both Composition and Options API on the README.
    • The Options API did need tour: null changed to tour: null as Shepherd.Tour|null for type inference to work correctly. Otherwise it gets inferred as only null.
  • An internal real-world Vue3 Options API codebase.
  • An internal real-world Vue3 Composition API codebase.

Additional Changes:

  1. I did have to run yarn upgrade (non-breaking) to get the project working on Node 20 workstations. A transitive dependency was limiting me to Node 18. The only direct dependency we had that got updated was eslint-plugin-vue, which just had a patch.
  2. Subsequently, I had to rebuild the /dist/ files via yarn build. Manual testing via test:e2e stills works.

What I did NOT do:

  1. Any linting/building/automation for Typescript - it's just hand crafted.
  2. Any extra shenanigans, like re-exporting the types from shepherd.js, so users will still need to import Shepherd from 'shepherd.js' in their projects when specifying types.

I wanted to keep these changes to the bare minimum.

Notes:

I've only done a minor version bump here. However, adding real types may be considered a breaking change for downstream projects. Please let me know if you want me to change it to a 4.0.0 release instead. 😄

With the Shepherd.js version requirement moving to ^12, this is now a bonafide breaking change.

Special thanks to @jim-toth and @losfroger for your comments on the related issue.

Some transitive dev dependencies in the lockfile were so old it locked
development to Node versions under 18. The current LTS is 20, so I figured
I'd `yarn upgrade` over switching to an old container.

Pretty much everything that got updated were transitive dependencies.
The only direct dependency was `eslint-plugin-vue`, which had a bug fix.
Thanks to @losfroger and @jim-toth in the related issue. This implementation
was a merge of their recommended fixes and the official Vue 3 Options API
Type Augmentation documentation.

I've tested the module definitions against a real-world Vue 3 codebase in both
Composition and Options API mode.

Closes #15.
@Kenneth-Sills
Copy link
Contributor Author

Kenneth-Sills commented Apr 9, 2024

For my own sanity, imported library as git+ through NPM in one of my projects. Confirmed that all the types are resolved just from package.json:types without needing a tsconfig.json of any kind. No shenanigans with npm link working, but npm install not. 😅

@Kenneth-Sills
Copy link
Contributor Author

Kenneth-Sills commented May 7, 2024

Will resolve conflicts once the review process starts. No rush to maintainer, but don't want to have to do it again if the dependabot changes go through before this. 😅

@Kenneth-Sills
Copy link
Contributor Author

@RobbieTheWagner Just a bump so this doesn't get lost. 😄

@RobbieTheWagner
Copy link
Member

@Kenneth-Sills can you please update this for Shepherd 12.0.4?

Kenneth-Sills and others added 3 commits May 28, 2024 10:15
Upstream's typing changed a bit, so I had to switch from namespace imports
over to "real" imports.

The distributable files have been rebuilt and the version bumped to the next
breaking change.

BREAKING CHANGE: Shepherd 12 is the new minimum, due to changes in typing.
Shepherd.js requires Node 18 or above, and yarn has had some bug fixes.
@Kenneth-Sills
Copy link
Contributor Author

Sorry for the delay, @RobbieTheWagner, didn't have much free time to get back to it today. I think this should be good to go, with two asterisks.

  1. The E2E tests for this project won't be able to run due to the upstream CSS asset issue (I do see you're fixing that here already). Since the workflow needs this to pass, we've got a blocker there.
  2. I also tried to re-verify the TS linting, but we added a dependency to idb@^8 upstream, which has a regression in its type specification (1, 2). The author isn't very active, so it may take some time for the proposed fixes to get merged in. Not a big deal - I still confirmed the updates are still correct in a real-world repo and our users should be running --skipLibCheck - but it does hamper the options for automated checks until this library is ported to TS proper or the idb maintainer has some extra time.

I ran AreTheTypesWrong and it does complain about the use of ESM without type: module, but that's really out of scope of this ticket. Otherwise, seems fine.

Thank you for your time, I appreciate it!

@Kenneth-Sills
Copy link
Contributor Author

Awesome, Shepherd 12.0.5 has been integrated into this, which should resolve (1) above.

@chuckcarpenter
Copy link
Member

@RobbieTheWagner is this merge ready then?

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR

@RobbieTheWagner RobbieTheWagner added the enhancement New feature or request label May 31, 2024
@RobbieTheWagner
Copy link
Member

It looks like the tests are choking on the CSS @Kenneth-Sills. Would you mind taking a look?

This should fix the failed imports when using Shepherd ^12.
@Kenneth-Sills
Copy link
Contributor Author

Of course, and I apologize for wasting your time not running test:e2e beforehand! 🤦

@josselinonduty
Copy link

Any updates on this?

@RobbieTheWagner
Copy link
Member

@Kenneth-Sills would you mind updating this to latest Shepherd and fixing the conflicts? Then I think we can merge.

@Kenneth-Sills
Copy link
Contributor Author

Kenneth-Sills commented Jun 15, 2024

@josselinonduty I've been out, recovering from a surgery. I'll likely have some time to get to this in the coming week or so!

EDIT: I went ahead and merged master. Passes yarn lint && yarn test:e2e.

@Kenneth-Sills
Copy link
Contributor Author

Kenneth-Sills commented Jun 18, 2024

@RobbieTheWagner I think we're going to need to coordinate a release, dependabot will keep invalidating my changes. 🤣 Just let me know what day and time would work best for you and I'll have the latest conflict resolved!

@chuckcarpenter chuckcarpenter merged commit 30abb7c into shipshapecode:master Jun 20, 2024
2 checks passed
@kfirprods
Copy link

@RobbieTheWagner @chuckcarpenter happy to see that this was merged :) can a new version be published though to get this through?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants