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.

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

[chore] prefer interfaces to types #2010

Merged
merged 1 commit into from
Jul 26, 2021
Merged

[chore] prefer interfaces to types #2010

merged 1 commit into from
Jul 26, 2021

Conversation

benmccann
Copy link
Member

I remember we had done this in Sapper. Makes sense to do it here too probably

@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 9275aab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann benmccann force-pushed the interfaces branch 2 times, most recently from 8068a47 to c87236a Compare July 25, 2021 14:32
@benbender
Copy link

@benmccann The big difference here is that interfaces can be extended while types can't. So mostly user-customizable interfaces and internal types is mostly the right rule of thumb!

@benmccann
Copy link
Member Author

@ignatiusmb you might be interested in this PR too given all the TypeScript stuff you've done. If it looks good to you or I don't hear anything I'll probably merge it in the next day or so

ignatiusmb
ignatiusmb reviewed Jul 26, 2021
View reviewed changes
Copy link
Member

@ignatiusmb ignatiusmb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍

Following a heuristic, we can prefer to use interface until we need to use features from type.

@benmccann benmccann merged commit d656316 into master Jul 26, 2021
@benmccann benmccann deleted the interfaces branch July 26, 2021 16:06
@ignatiusmb ignatiusmb mentioned this pull request Jul 26, 2021
Merged
@jthegedus
Copy link
Contributor

jthegedus commented Jul 26, 2021 •
edited
Loading

I'm certainly no TS expert, but as I understand the only remaining difference in the latest TypeScript version is that interface supports implicit open typing (merging) whereas type will error - https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#differences-between-type-aliases-and-interfaces

Seems to me type would then be less error prone as extending the type def would always need to be explicit.

Again, no TS expert.

@JeanJPNM
Copy link
Contributor

I think interfaces give some flexibility when it comes to customization, being able to extend types without having to create new ones can be useful in some situations.

@jthegedus
Copy link
Contributor

jthegedus commented Jul 27, 2021 •
edited
Loading

I think interfaces give some flexibility when it comes to customization, being able to extend types without having to create new ones can be useful in some situations.

For sure, in some situations. Just seems like a huge footgun. I see this as changing from const to var because extending a const value is useful sometimes. Seems overkill to make all const values vars.

My point was just that type in TypeScript has changed a lot to be near parity to interface and perhaps the reasoning to use one over the other in the Sapper repo is no longer applicable here.

@JeanJPNM
Copy link
Contributor

Yeah, but you can only extend an interface by redeclaring it (sort of), and if people take the time to redeclare an interface they usually know what they are doing

sidharthv96 added a commit to sidharthv96/kit that referenced this pull request Jul 31, 2021
@sidharthv96
Merge branch 'master' of https://github.com/sveltejs/kit into fix/bas…
d1179e2 
…ePath

* 'master' of https://github.com/sveltejs/kit: (85 commits)
  [chore] minor simplification of `parse_body` (sveltejs#2043)
  Version Packages (next) (sveltejs#2026)
  [chore] deduplicate config types (sveltejs#2030)
  [chore] remove invalid css declaration (sveltejs#2038)
  [fix] correctly pass Vite options in preview mode (sveltejs#2036)
  [feat] Ignore adapter build files (sveltejs#1924)
  [chore] Upgrade Rollup
  Don't check external links on prerender (sveltejs#1679)
  Version Packages (next) (sveltejs#2017)
  [chore] convert remaining type aliases (sveltejs#2018)
  [feat] explicitly set compilerOptions.hydratable to config.kit.hydrate (sveltejs#2024)
  [feat] More powerful and configurable rendering options (sveltejs#2008)
  [chore] typecheck example (sveltejs#2019)
  Change config `prerender.force` to `prerender.onError` (sveltejs#2007)
  [chore] prefer interfaces to types (sveltejs#2010)
  [docs] minor wording improvement in migration guide (sveltejs#2006)
  [chore] test debugging improvements
  [docs] fix typo (sveltejs#2003)
  [chore] use @ts-expect-error instead of @ts-ignore (sveltejs#1999)
  Version Packages (next) (sveltejs#1996)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignatiusmb ignatiusmb ignatiusmb left review comments

Assignees
No one assigned
Labels
types / typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@benmccann @benbender @jthegedus @JeanJPNM @ignatiusmb