Skip to content

refactor: remove tsd integration #392

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.

Sign up for GitHub

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

Jump to bottom
Merged
mcataford merged 13 commits into from
Sep 16, 2021
Merged

refactor: remove tsd integration #392

mcataford merged 13 commits into from
Sep 16, 2021

Conversation

@andrewsantarin
Copy link
Contributor

@andrewsantarin andrewsantarin commented Sep 12, 2021 •
edited
Loading

This PR discards the usage of tsd in tests because it has very limited use in this library at the moment.

Resolves #389 (comment)

Description

  • Removed tsd usage, dependency and mention.
  • Removed "TypeScript usage" from the doc to simplify readability.
  • Improved linting in TypeScript tests.

Screenshots (if appropriate):

Not required. This is a development workflow change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have used ESLint & Prettier to follow the code style of this project.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@andrewsantarin andrewsantarin mentioned this pull request Sep 12, 2021
Merged
7 tasks
@andrewsantarin andrewsantarin force-pushed the feature/remove-tsd-integration branch from 4d2dff3 to 31cb5ae Compare September 12, 2021 07:44
@andrewsantarin andrewsantarin force-pushed the feature/remove-tsd-integration branch from b7b91a8 to ec7bc53 Compare September 12, 2021 10:55
mcataford
mcataford reviewed Sep 12, 2021
View reviewed changes
Copy link
Collaborator

@mcataford mcataford left a comment

Choose a reason for hiding this comment

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

Some testing thoughts. I'll spend a bit of time later looking at consolidating the ts/js tests so they are all captured by the runner & coverage reports, but in the meantime, the couple changes suggested should trim off some more-or-less dead code.

Thank you for following up on the tsd refactor! 🚀

@andrewsantarin
Copy link
Contributor Author

andrewsantarin commented Sep 16, 2021 •
edited
Loading

One other thing:

The point of having additional TypeScript tests was to ensure that the declarations worked just as advertised. I do realize that this could be overkill for a library that is still written in plain JavaScript. I rushed quickly into ensuring that the TypeScript side doesn't go to public with at least some quality assurance.

So, in the spirit of keeping things lean, perhaps just dropping the .test.ts files altogether sounds about good right now.

consolidating the ts/js tests

If we had to introduce TypeScript to tests here, some supplementary (NOT replacement!) tooling for it is in order, e.g. ts-jest. I do use this at work and it has worked pretty well so far. We don't have to address it right here right now if this is a huge scope creep. I'm glad that we made it to official TypeScript support at this rate.

@mcataford
Copy link
Collaborator

mcataford commented Sep 16, 2021

100% on ts-jest. Let's do that as a follow up!

mcataford
mcataford approved these changes Sep 16, 2021
View reviewed changes
Copy link
Collaborator

@mcataford mcataford left a comment

Choose a reason for hiding this comment

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

LGTM as-is -- the getAllCountries test can wait, I've got some changes that might fill those gaps.

@mcataford mcataford merged commit 7ee9921 into patw0929:master Sep 16, 2021
@andrewsantarin andrewsantarin deleted the feature/remove-tsd-integration branch September 16, 2021 14:40
@github-actions
Copy link

github-actions bot commented Sep 22, 2021

🎉 This PR is included in version 8.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

andrewsantarin added a commit to andrewsantarin/react-intl-tel-input that referenced this pull request Feb 2, 2022
@andrewsantarin
refactor: remove tsd integration (patw0929#392)
fae1b57 
* refactor: remove tsd usage

* build: remove tsd configuration

* docs: remove tsd mention

* docs: remove complicated TypeScript usage

These days, I imagine it's commonly understood how imports in TypeScript work.

* chore: allow JSX in TSX

* chore: apply eslint config to all TypeScript files

* chore: allow console logging in TypeScript tests

* chore: use multiline JSX props in TSX

* refactor: satisfy eslint and prettier in TypeScript tests

* build: bump typescript to 4.4.3

* refactor: remove unnecesary constants test

* test: fix getCountryData object equality assertion

* test: transfer getCountryData TS tests to JS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcataford mcataford mcataford approved these changes

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andrewsantarin @mcataford