feat(vue): add typescript support#1920
Conversation
|
hi @jackocnr friendly ping :) |
|
This looks amazing, thank you so much! A few questions/comments:
Again, thank you so much for this great work 💐 |
The problem is that the file and category names are the same, which may cause conflicts. what about |
|
Re: providing build files, I suppose we don't need to provide build files in the standard case e.g. if your project uses a build tool that supports TypeScript you can just import the Vue src file. The problem is for someone who is not using TS, and doesn't support it in their current build setup - how will they import the component? Or am I overthinking this? Do all Vue bundlers/build tools support TS by default? Re: the exports section in package.json, now I'm wondering: what's the point in adding the build paths here, when we don't provide those build files when the package is consumed? Again, I fear I'm missing something here - my understanding of these things might be flawed or out of date - if that's the case, forgive me! Re: future TODOs - I'm open to all of these ideas, but can you help me understand the benefits of each one? Perhaps it's better to discuss these ideas separately. Thanks again. |
but we build this before publishing and end up with pure js with no typscript only .d.ts types but next to the js files
exports it is just a wrapper for vue or more specifically for the d.ts plugin which needs an input file, it is just a feature of building with this solution. |
|
I would really love to get this merged, but sadly don't know anything about Vue. Perhaps our beloved Vue team could offer their opinions on how to get this over the line? @carlssonemil @joaovinicius @mdpoulter @bettysteger 🙏🏻 |
carlssonemil
left a comment
There was a problem hiding this comment.
As I rarely use TypeScript (we don't use it at work), take my opinion with a grain of salt.
I don't see any big issues with this, other than what you two have already discussed regarding the potential conflict with the module names. It will in the end modernize the library and add type-safe exports for the Vue component.
As long as we can make sure that it does not mess with the module name and the exports are accessible after importing the component, we should be good. We probably also need to update the documentation to reflect these changes.
- need repo convert to monorepo (for clean installations for different frameworks, better ts support).
This would probably help a lot with separating the code bases and make it easier to maintain a specific framework. But, it might also add a lot of unwanted overhead when maintaining 🤷
If you're hesitant with the file re-structure, which is warranted, it's entirely up to you if you want to go forward with these changes. As someone that does not really need TypeScript in this package, I don't really mind whichever way you decide to go 😁
| value: { | ||
| type: String, | ||
| default: "", | ||
| }, |
There was a problem hiding this comment.
This change is good, but I'm pretty sure it will be a breaking change since you would now need to pass your local value with the v-model attribute rather than the value attribute. So everyone updating to this version will need to update their value handling.
<!-- Current value handling -->
<IntlTelInput :value="localValue" />
<!-- Updated value handling with these changes -->
<IntlTelInput v-model="localValue" />I'd much rather we do this more gracefully. For example, we can still do this change, but we keep the value prop and set the number to whichever prop (v-model or value) contains a value. And then plan to deprecate the value prop later on. Or, we just do this change and document it as a breaking change in the release notes.
There was a problem hiding this comment.
Thanks for spotting this. I've since taken @reslear's commit, rebased it on master, added a couple of fixes, and pushed it to a new branch called vue-ts. Is there any chance you would be up for making this change you're proposing, to prevent this from being a breaking change, and creating a PR to push it to that branch? Then I think we can release this!
There was a problem hiding this comment.
Thanks for spotting this. I've since taken @reslear's commit, rebased it on master, added a couple of fixes, and pushed it to a new branch called vue-ts. Is there any chance you would be up for making this change you're proposing, to prevent this from being a breaking change, and creating a PR to push it to that branch? Then I think we can release this!
Definitely! I'll see if I can find the time this week to put up a PR!
There was a problem hiding this comment.
By the way, if there is an option to simply undo this change to get this TS support merged, then that'd be fine with me as well! Then we can think about re-adding it in the next major release. I'm not sure if that's possible tho... I'll leave it to you!
There was a problem hiding this comment.
Also, I've pushed a few fixes to master - do you mind if I rebase the vue-ts branch and force push? I don't want to complicate things if you're already working on this?
There was a problem hiding this comment.
I haven't found the time to start yet, so feel free to rebase! I'll do my best to get it done in the coming days!
|
Thanks so much for your thoughts here @carlssonemil 💐
After reflecting on this a bit longer, I think this will all be fine TBH. I say let's get this merged, as long as we can make it a non-breaking change for now (see my other comment). |
|
do you need help from me? |
If you feel like you have the time, then yes! I'm completely swamped with work this week. It should be a pretty easy change, just make sure you define the local |
|
Please be sure to rebase when you make any changes, as I've since deleted all build files from the project. |
|
I sincerely apologize for not fixing the |
|
Hi guys, thanks for your work on this, I've already merged this into my new v26 branch, hoping to release soon! I'll post here when I do. |
|
Released in v26.0.0-beta.0 - @carlssonemil @reslear please try importing this version into your projects and test if the TypeScript types work 🤞🏻 (please see the list of breaking changes in this release in the linked release notes) |
| "src/intl-tel-input/i18n/**/*.ts", | ||
| "src/intl-tel-input/utils-compiled.js", | ||
| "src/intl-tel-input/intlTelInputWithUtils.ts", | ||
| "src/intl-tel-input/data.ts", |
There was a problem hiding this comment.
Hi @reslear, I'm not familiar with Vue myself, but I've just created a new test project with create-vue, with typescript support, and imported this component, and the type checking worked, which is great news!
With that said, I found a couple of things that didn't work, e.g. I could set the initialCountry option to an invalid country code like "gbbb" and it didn't give an error. Or if I use the i18n option, I could pass invalid translation keys without seeing an error. I looked in the type definition files, and they seem to have broken links to intl-tel-input/data and files in intl-tel-input/i18n/ which I then traced back to these exclude lines.
I've just done a test and commented out (all of) these excludes, and released that as v26.0.0-beta.1, and that fixes the issues for me (I now get a TS error if I try to use an invalid country code or translation key). But I just wanted to check in with you about this change - was there a reason for excluding these files in the first place?

resolve: #1794
hi @jackocnr pls review :)
Changes:
Typed
input-propsandoptionsboth components types support:
exportable
SomeOptionsnot necessarily, but maybe in the future:use
modelValueinsteadvalueforv-modelused vite multiple entries instead double run vite with custom configs.
TODO on future:
pnpmwith worksapces, and move build data toshared packageinstead symlinks.grunte.g zx/vueWithUtils-> kebab-case e.g/vue-with-utils