Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

use typescript native types #2168

Merged
merged 1 commit into from
Feb 23, 2022
Merged

use typescript native types #2168

merged 1 commit into from
Feb 23, 2022
+9 −29

Conversation

m4thieulavoie
Copy link
Contributor

@m4thieulavoie m4thieulavoie commented Feb 9, 2022 •
edited by BPScott
Loading

Description

This PR removes ThenType, Omit, Arguments, ArgumentAtIndex, FirstArgument, MaybeFunctionReturnType, ConstructorArguments, ConstructorArgumentAtIndex and FirstConstructorArgument as can all be trivially replaced by built-in typescript types (Awaited, Omit, Parameters, ConstructorParameters, ReturnType)

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

@m4thieulavoie m4thieulavoie requested a review from BPScott February 9, 2022 21:10
@m4thieulavoie m4thieulavoie self-assigned this Feb 9, 2022
@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch from e58e87b to 6463ec4 Compare February 9, 2022 21:13
Base automatically changed from use-native-typescript-types to main February 10, 2022 13:20
@m4thieulavoie m4thieulavoie marked this pull request as ready for review February 10, 2022 13:21
@m4thieulavoie m4thieulavoie requested a review from a team as a code owner February 10, 2022 13:21
BPScott
BPScott reviewed Feb 10, 2022
View reviewed changes
packages/useful-types/CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
### Added

- Added `PartialSome` and `RequireSome` types to set specified fields of a property to optional or required
- Deprecate `ThenType`, `Omit`, `Arguments`, `ArgumentAtIndex`, `FirstArgument`, `MaybeFunctionReturnType`, `ConstructorArguments`, `ConstructorArgumentAtIndex` and `FirstConstructorArgument` in favour of their Typescript counterparts
Copy link
Member

Choose a reason for hiding this comment

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

We've already released 3.1.0, this information needs to go under the unreleased heading, with the subheading ### Breaking Change as this is breaking behaviour.

This isn't a deprecation - deprecations are "this behaviour still exists but you shouldn't use it", this is full on removal of features.

Please write some migration notes that tells consumers what they should use instead these now removed functions ("Replace usage of Arguments with the built-in Parameters" kinda thing. Linking to specific sections of the TS docs would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Where should I write those docs? Directly in here?

Copy link
Member

Choose a reason for hiding this comment

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

Yep! So there's a section up above that says <!-- Unreleased -->, directly under that add the new breaking change heading, and then add a bullet point for each thing that has a breaking change, and an explanation of what to do instead

@BPScott
Copy link
Member

BPScott commented Feb 12, 2022

CI failures due to shifting node versions fixed in #2171. Rebase and CI should be happy again

@m4thieulavoie m4thieulavoie force-pushed the remove-homemade-types branch 2 times, most recently from c622eb9 to d2a9637 Compare February 15, 2022 18:36
@m4thieulavoie m4thieulavoie force-pushed the remove-homemade-types branch 2 times, most recently from 5dda883 to 2c91049 Compare February 22, 2022 20:46
BPScott
BPScott reviewed Feb 22, 2022
View reviewed changes
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Code looks good. Dropped some thoughts inline regarding changlog content.

packages/useful-types/CHANGELOG.md Outdated Show resolved Hide resolved
packages/useful-types/CHANGELOG.md Outdated
## 3.1.0 - 2022-02-09

### Added

- Added `PartialSome` and `RequireSome` types to set specified fields of a property to optional or required
- Deprecate `ThenType`, `Omit`, `Arguments`, `ArgumentAtIndex`, `FirstArgument`, `MaybeFunctionReturnType`, `ConstructorArguments`, `ConstructorArgumentAtIndex` and `FirstConstructorArgument` in favour of their Typescript counterparts
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this line in the changelog of 3.1.0 - we never marked these types with @deprecated or any other mechanism. I reckon we should remove it.

BPScott
BPScott approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Thanks :shipit:

@m4thieulavoie
use typescript native types
994a056 
changelog

remove homemade types

also this one

reword changelog

add breaking changes to changelog

Update packages/useful-types/CHANGELOG.md

Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>

pr comments

fix ci
@m4thieulavoie m4thieulavoie merged commit e344c08 into main Feb 23, 2022
@m4thieulavoie m4thieulavoie deleted the remove-homemade-types branch February 23, 2022 19:29
@shopify-shipit shopify-shipit bot temporarily deployed to a11y_tests_class_improvement February 24, 2022 21:31 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production February 25, 2022 22:56 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to mf-beta March 7, 2022 09:18 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production-gem June 21, 2022 14:45 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BPScott BPScott BPScott approved these changes

@ryanwilsonperkin ryanwilsonperkin Awaiting requested review from ryanwilsonperkin

Assignees

@m4thieulavoie m4thieulavoie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@m4thieulavoie @BPScott @ryanwilsonperkin