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

Fix type issue with the latest frontend-shared version #3738

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

esanzgar
Copy link
Contributor

@esanzgar esanzgar commented Sep 6, 2021

In the latest version of the frontent-shared package, extraRoute has a
mandatory property group that must be either
"home" | "foundations" | "patterns" | "components";

There are three ways to fix this:

  • make the group property optional in the frontend-shared package (I
    don't know if that's a good idea).

  • cast the string type to one of the allowed types: group: /** @type {'components'} */ ('components'),

  • convert the string type to const without casting (this PR).

@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #3738 (2e5a0bd) into master (c91cbf0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3738   +/-   ##
=======================================
  Coverage   98.95%   98.95%           
=======================================
  Files         211      211           
  Lines        7671     7671           
  Branches     1731     1731           
=======================================
  Hits         7591     7591           
  Misses         80       80           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 223f329...2e5a0bd. Read the comment docs.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

I suggest moving the cast to a different place, both to reduce the amount of annotations needed in future and to get more helpful error messages.

@@ -3,11 +3,15 @@ import ButtonPatterns from './components/ButtonPatterns';

import sidebarIcons from '../../src/sidebar/icons';

/** @type {import('@hypothesis/frontend-shared/lib/pattern-library').PlaygroundRoute['group']} */
Copy link
Member

@robertknight robertknight Sep 6, 2021

Choose a reason for hiding this comment

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

It'll be easier to add the cast to the extraRoute definition:

/** @type {import('@hypothesis/frontend-shared/lib/pattern-library').PlaygroundRoute[]} */
const extraRoutes = [
  {
    route: '/buttons',
    title: 'Buttons',
    component: ButtonPatterns,
    group: 'components',
  },
];

This way you only have to specify a type once if there are multiple fields like group. You'll also get an earlier error if the extraRoutes object does not have the intended structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks.

An alternative approach that will be available with typescript 4.5 is:

group: /** @type {const} */ ('components')

microsoft/TypeScript#30445

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's useful to know. I think I'd still prefer to type extraRoutes in this particular case in order to communicate intent to readers.

In the latest version of the frontent-shared package, `extraRoute` has a
mandatory property `group` that must be either
`"home" | "foundations" | "patterns" | "components"`;

There are three ways to fix this:

* make the `group` property optional in the frontend-shared package (I
  don't know if that's a good idea).

* cast the `string` type to the one of the allowed types: `    group:
  /** @type {'components'} */ ('components'),`

* cast the `extraRoute` (this PR).
@esanzgar esanzgar force-pushed the fix-type-issue-with-frontend-shared branch from 9008aa8 to 2e5a0bd Compare September 7, 2021 08:30
Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

I think this change is fine and innocuous for the moment.

I think it would likely be more ergonomic for authors in the future if the package weren't so persnickity about groups and their type. Perhaps a sensible default fallback for extraRoutes that lack a group? I'll track this (in frontend-shared).

This linting grumpiness is just popping up now in the newest version of the package, which suggests that this is a side effect of the updated type-declaration generation? That is, typing for this hasn't been touched directly in some time. Any insight you might have on that would be helpful!

@esanzgar
Copy link
Contributor Author

esanzgar commented Sep 8, 2021

@lyzadanger, you are right this is caused by changes in frontend-shared@3.8.1.

Previously, only JS files that were 'linked' to src/index.js had type definitions. This is because the rule to create types: tsc --allowJs --declaration --emitDeclarationOnly --outDir lib src/index.js. That caused the pattern-library to not have any typings.

In version 3.8.1, type definitions are generated for all JS files: "include": ["**/*.js"], based on the tsconfig.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants