Skip to content
This repository was archived by the owner on Mar 14, 2024. It is now read-only.

feat(typing): add TypeScript typings and lint types, fixes #2720#3222

Merged
robdodson merged 13 commits intomasterfrom
feat/typing
Aug 22, 2020
Merged

feat(typing): add TypeScript typings and lint types, fixes #2720#3222
robdodson merged 13 commits intomasterfrom
feat/typing

Conversation

@MichaelSolati
Copy link
Copy Markdown
Contributor

@MichaelSolati MichaelSolati commented Jun 13, 2020

Fixes #2720

  • Add npm script to add type checking of JSDoc via typescript.
  • Add types folder with d.ts files for global typescript definitions of objects.
  • Corrected typings in JSDoc syntax for files and variables.
  • Added typings in JSDoc syntax for files and variables.
  • Renamed the description field in _data/authorData.json to descriptions to allow for collection definition to extend data definition. It also made sense as it is designed to support multiple languages (hence multiple descriptions).
  • Add optional chaining for DOM elements from query selectors in order to confirm that they exist cleanly, rather than writing if statements.
  • Add some properties of custom elements to be defined in the constructor in order to allow for use in methods in order to prevent the compiler saying the property doesn't exist on the object.

@MichaelSolati MichaelSolati added the DO NOT MERGE Actively working on but experimental label Jun 13, 2020
@googlebot googlebot added the cla: yes Contributor has signed the CLA label Jun 13, 2020
@netlify
Copy link
Copy Markdown

netlify bot commented Jun 13, 2020

Deploy preview for web-dev-staging ready!

Built with commit 057b4c0

https://deploy-preview-3222--web-dev-staging.netlify.app

@stale
Copy link
Copy Markdown

stale bot commented Jul 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. To prevent this from happening, leave a comment.

@stale stale bot added the stale label Jul 13, 2020
@stale stale bot closed this Jul 20, 2020
@robdodson
Copy link
Copy Markdown
Contributor

@MichaelSolati did you mean for this to close?

@MichaelSolati
Copy link
Copy Markdown
Contributor Author

No.. i let it sit to long...

@MichaelSolati MichaelSolati reopened this Jul 21, 2020
@stale stale bot removed the stale label Jul 21, 2020
@MichaelSolati MichaelSolati requested a review from a team as a code owner July 23, 2020 18:06
@MichaelSolati MichaelSolati requested a review from devnook July 23, 2020 18:06
@MichaelSolati MichaelSolati force-pushed the feat/typing branch 3 times, most recently from 7266a87 to 5e4042b Compare July 28, 2020 19:36
@MichaelSolati MichaelSolati removed the DO NOT MERGE Actively working on but experimental label Jul 31, 2020
@MichaelSolati
Copy link
Copy Markdown
Contributor Author

MichaelSolati commented Jul 31, 2020

I've kept losing stuff and falling behind on keeping this up to date with the main branch that I thought we could just bring this in as it is now. Not all issues are resolved (about 185 typing issues exist), but it doesn't include the type linting in the lint command, so we can passively fix them as we change code. Also issues will still be highlighted in VS Code, but it wont fail our linting tests.

I do need to go through the changes I've made and fix some stuff, but I think this could/should be wrapped up soon.

@MichaelSolati MichaelSolati requested a review from a team as a code owner July 31, 2020 20:18
@MichaelSolati MichaelSolati changed the title [WIP] feat(typing): add TypeScript typings and lint types, fixes #2720 feat(typing): add TypeScript typings and lint types, fixes #2720 Jul 31, 2020
@MichaelSolati
Copy link
Copy Markdown
Contributor Author

Looks like GitHub actions is freezing up again, anyway...

@robdodson and @samthor can you PTAL, lots of small changes which shouldn't break anything but should require scrutiny.

@kaycebasques and @jpmedley I believe you've been tagged in because i renamed description to descriptions in the _data/authorData.json. Reason for this was because I created a type definition for the objects in the _data/authorData.json as well as the _collections/authors.js, the collection item extended the data item, but I could not overwrite the type of the item i was extending. It was easier to rename the description field in _data/authorData.json, and it also made sense as it is designed to support multiple languages (hence multiple descriptions).

@robdodson
Copy link
Copy Markdown
Contributor

I think this PR also added a babel plugin for optional chaining which seems like an important addition to call out.

I might prefer to add that in a separate PR and maybe we discuss the ramifications first. My main concern is if it makes it so things start silently failing where previously they caused errors that we could track down and file as bugs.

Copy link
Copy Markdown
Contributor

@robdodson robdodson left a comment

Choose a reason for hiding this comment

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

Looks good. Left a number of questions but I think we can land it tomorrow

*/

declare global {
export interface WMouseEvent<T extends HTMLElement> extends MouseEvent {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Initially I thought this was referring to a mouse Wheel event.

Maybe the type could hint at why it's needed. Like MouseEventWithTarget or something like that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was confused too but I actually really like this type. It seems like an obvious addition to Event.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh yeah I'm cool with adding the type, but I'm wondering if its name can be more descriptive. At a glance it's not clear what WMouseEvent means.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went with W as we use it as a prefix for our classes. I can rename it if there's a preference.

Copy link
Copy Markdown
Contributor

@samthor samthor left a comment

Choose a reason for hiding this comment

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

Sorry to pile on the nit-picks. I am really excited for this PR generally.

I am concerned about .?, I think we can make TS happy without it, but I accept that it could be quite difficult since the lifecycle of custom elements isn't great (their sub-elements will be null a lot of the time, while disconnected).

*/

declare global {
export interface WMouseEvent<T extends HTMLElement> extends MouseEvent {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was confused too but I actually really like this type. It seems like an obvious addition to Event.

@github-actions
Copy link
Copy Markdown

Alex Recommends Report

Alex recommends the following language changes, but Alex is a regular expression based algorithm, so take them with a grain of salt.

src/site/content/en/handbook/author-profile/index.md

Level Location Word Recommendation
⚠️ 67:36 clearly clearly may be insensitive, try not to use it

if (changedProperties.has('switching') && this.switching) {
input.setSelectionRange(0, input.value.length);
input.focus();
input?.setSelectionRange(0, input?.value.length);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ping

@robdodson robdodson merged commit 37f10fe into master Aug 22, 2020
@robdodson robdodson deleted the feat/typing branch August 22, 2020 00:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Contributor has signed the CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add typescript comments and enforce with tsc compiler.

4 participants