vue-shepherd
vue-shepherd copied to clipboard
shipshapecode
Add Native Typescript Declarations
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: nullchanged totour: null as Shepherd.Tour|nullfor type inference to work correctly. Otherwise it gets inferred as onlynull.
- The Options API did need
- An internal real-world Vue3 Options API codebase.
- An internal real-world Vue3 Composition API codebase.
Additional Changes:
- 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 waseslint-plugin-vue, which just had a patch. - Subsequently, I had to rebuild the
/dist/files viayarn build. Manual testing viatest:e2estills works.
What I did NOT do:
- Any linting/building/automation for Typescript - it's just hand crafted.
- Any extra shenanigans, like re-exporting the types from
shepherd.js, so users will still need toimport 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. :smile:
Special thanks to @jim-toth and @losfroger for your comments on the related issue.
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. :sweat_smile:
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. :sweat_smile:
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.
- 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.
- I also tried to re-verify the TS linting, but we added a dependency to
idb@^8upstream, 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 theidbmaintainer 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!
It looks like the tests are choking on the CSS @Kenneth-Sills. Would you mind taking a look?
Of course, and I apologize for wasting your time not running test:e2e beforehand! 🤦
@Kenneth-Sills would you mind updating this to latest Shepherd and fixing the conflicts? Then I think we can merge.
@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.
@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!
@RobbieTheWagner @chuckcarpenter happy to see that this was merged :) can a new version be published though to get this through?
