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.

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

refactor(tsconfig): enable verbatimModuleSyntax #4237

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

joshblack
Copy link
Member

Based on changes over in: https://github.com/github/github/pull/293430

This PR enables the verbatimModuleSyntax option in our base tsconfig. This change prevents some of the challenges we were running into where Gatsby or Storybook were reporting issues importing/finding certain imports that were using type elision (where TS detects that you're importing a type but bundlers may not be aware that it is a type).

Note: this still allows mixed imports as long as type is included before the named import. For example:

import { ExampleExport, type ExampleExportProps } from './example';

Changelog

New

Changed

  • Enable verbatimModuleSyntax option in tsconfig.base.json
  • Enable eslint rule for this syntax
  • Run autofix for eslint rule to refactor imports

Removed

Rollout strategy

  • None; if selected, include a brief description as to why

I believe no public change should be perceivable through this refactor

Testing & Reviewing

  • Would love to hear what folks think of enabling this rule and its impact on PRC
  • Are there any downsides to enabling this rule?

@joshblack joshblack requested review from a team and siddharthkp February 8, 2024 18:57
Copy link

changeset-bot bot commented Feb 8, 2024

⚠️ No Changeset found

Latest commit: b2802d2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@joshblack joshblack added the skip changeset This change does not need a changelog label Feb 8, 2024
Copy link
Contributor

github-actions bot commented Feb 8, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 113.37 KB (0%)
packages/react/dist/browser.umd.js 114.03 KB (0%)

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

TIL!

I don't know the tradeoffs of this, but trust you with it!

Just confirming, I understand this does not change how types can imported outside the package, right?

@joshblack
Copy link
Member Author

@siddharthkp correct! Should be no change at all downstream. I think that, for us, the only drawback is that we have to explicitly annotate type imports.

@joshblack joshblack added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit 49c6f18 Feb 14, 2024
30 checks passed
@joshblack joshblack deleted the refactor/add-verbatim-module-syntax branch February 14, 2024 16:56
broccolinisoup pushed a commit that referenced this pull request Feb 20, 2024
* refactor(tsconfig): enable verbatimModuleSyntax

* refactor(ts): eslint autofix verbatimModuleSyntax

* chore: run formatting

* chore(eslint): apply autofix

* fix: update type import syntax

---------

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
* refactor(tsconfig): enable verbatimModuleSyntax

* refactor(ts): eslint autofix verbatimModuleSyntax

* chore: run formatting

* chore(eslint): apply autofix

* fix: update type import syntax

---------

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants