Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

VHeaderFilter Typescript Conversion #1753

Closed
1 task done
ramadanomar opened this issue Aug 29, 2022 · 7 comments
Closed
1 task done

VHeaderFilter Typescript Conversion #1753

ramadanomar opened this issue Aug 29, 2022 · 7 comments
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon 🚧 status: blocked Blocked & therefore, not ready for work 🟨 tech: javascript Requires familiarity with JavaScript ⌨️ tech: typescript Requires familiarity with TypeScript
Projects

Comments

@ramadanomar
Copy link
Contributor

Description

The types in VHeaderFilter aren't detected even though they are defined in ~/composables

Screenshots

image

Additional context

Stumbled upon this while trying to figure out why tests results vary depending on the enviroment #1737. This error gets replaced by others if we include /src/components/VHeader/VHeaderFilter.vue to the tsconfig file.
image

I suspect it is related to #1752. I am working on finding a fix. If you have any insight / ideas on how to approach this, i'd greatly appreciate it !

Resolution

  • 🙋 I would be interested in resolving this bug.
@ramadanomar ramadanomar added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository 🟨 tech: javascript Requires familiarity with JavaScript ⌨️ tech: typescript Requires familiarity with TypeScript labels Aug 29, 2022
@ramadanomar ramadanomar self-assigned this Aug 29, 2022
@openverse-bot openverse-bot added this to Backlog in Openverse Aug 29, 2022
@obulat
Copy link
Contributor

obulat commented Aug 29, 2022

Have you run pnpm install after checking out the PR (or are you running into these errors on main), @ramadanomar? The version of vue-tsc has been updated recently, and different versions produce different type errors.

Also, there are currently several open PRs that update the types and modals, it might be better to wait until they land to try to figure out what causes this issue, if there are still any left. The biggest simplification related to the issues in your screenshot is in #1751, and it should fix them.

@ramadanomar
Copy link
Contributor Author

Okay! I'll wait for the new PRs to be merged until continuing with this issue.

@ramadanomar ramadanomar added the 🚧 status: blocked Blocked & therefore, not ready for work label Aug 29, 2022
@obulat
Copy link
Contributor

obulat commented Aug 29, 2022

I tried looking into it, and it appears to be a VSCode/Volar problem. I don't see these problems in WebStorm, and you don't get errors when you run pnpm types.

The problem seems to be that VSCode or Volar are not picking up the paths object in the tsconfig file, and marking all the aliased paths that start with ~ as not found. If you replace all of the aliased paths with the relative paths ( '~/utils/focus-management' with '../../utils/focus-management'), the problem disappears.

@ramadanomar
Copy link
Contributor Author

The problem seems to be that VSCode or Volar are not picking up the paths object in the tsconfig file

I think it's because we don't include it in tsconfig.json

{ /* ... */
"include": [
  "**/*.ts",
/*rest of the files */
  "src/components/VHeader/VHeaderFilter.vue",
 ],
 }

Adding something like that to tsconfig.json fixes the problem but we get some type errors "Type HTMLElement | null " is not assignable. Weird thing is it doesn't fail the pnpm test linting check.

@ramadanomar
Copy link
Contributor Author

Nvm it fails when i try to upload a branch.

@obulat
Copy link
Contributor

obulat commented Aug 30, 2022

@ramadanomar, I've added lang="ts" to the VHeader components to be able to use TypeScript when writing them. I did not add them to tsconfig so that they wouldn't raise TypeScript errors (see more details in this comment). Normally, this can raise warnings in the IDE, but should not cause errors.

We have vendored-in the Reakit functions, and I plan to convert VHeader, VModal and VPopover components to valid Typescript and add them to TSConfig after the modal refactor PRs are merged in.

@ramadanomar
Copy link
Contributor Author

Oh, i understand. I'll close this issue as it behaves normally then. Lmk if i can help with the typescript conversion

Openverse automation moved this from Backlog to Done! Aug 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon 🚧 status: blocked Blocked & therefore, not ready for work 🟨 tech: javascript Requires familiarity with JavaScript ⌨️ tech: typescript Requires familiarity with TypeScript
Projects
No open projects
Openverse
  
Done!
Development

No branches or pull requests

2 participants