feature/feat-improve-ts-setup #1812

Merged
dpschen merged 10 commits from dpschen/frontend:feature/feat-improve-ts-setup into main 2022-07-04 21:50:50 +00:00
Member

This implements the vue ts setup by https://github.com/vuejs/create-vue

This implements the vue ts setup by https://github.com/vuejs/create-vue
dpschen added a new dependency 2022-04-11 20:18:55 +00:00
dpschen force-pushed feature/feat-improve-ts-setup from fe5e79a90a to ece7f39623 2022-04-11 20:24:12 +00:00 Compare
dpschen force-pushed feature/feat-improve-ts-setup from ece7f39623 to 9cdb507ace 2022-04-11 20:38:32 +00:00 Compare
dpschen force-pushed feature/feat-improve-ts-setup from 9cdb507ace to 41123d7bc1 2022-04-11 20:40:09 +00:00 Compare
Author
Member

I guess this is already much better.

@konrad: any idea how to disable ts from accessing the drone ts stuff:

+ yarn run lint
yarn run v1.22.18
$ eslint --ignore-pattern '*.test.*' ./src --ext .vue,.js,.ts

/drone/src/src/indexes/index.ts
  1:19  error  'SimpleDocumentSearchResultSetUnit' is defined but never used  @typescript-eslint/no-unused-vars

/drone/src/src/main.ts
  87:33  error  'vm' is defined but never used    @typescript-eslint/no-unused-vars
  87:37  error  'info' is defined but never used  @typescript-eslint/no-unused-vars
  97:33  error  'vm' is defined but never used    @typescript-eslint/no-unused-vars
  97:37  error  'info' is defined but never used  @typescript-eslint/no-unused-vars

/drone/src/src/types/shims-vue.d.ts
  12:10  error  'ComponentCustomProperties' is defined but never used  @typescript-eslint/no-unused-vars

/drone/src/src/views/tasks/ShowTasks.vue
  94:11  error  'key' is defined but never used  @typescript-eslint/no-unused-vars

✖ 7 problems (7 errors, 0 warnings)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
I guess this is already much better. @konrad: any idea how to disable ts from accessing the drone ts stuff: ``` + yarn run lint yarn run v1.22.18 $ eslint --ignore-pattern '*.test.*' ./src --ext .vue,.js,.ts /drone/src/src/indexes/index.ts 1:19 error 'SimpleDocumentSearchResultSetUnit' is defined but never used @typescript-eslint/no-unused-vars /drone/src/src/main.ts 87:33 error 'vm' is defined but never used @typescript-eslint/no-unused-vars 87:37 error 'info' is defined but never used @typescript-eslint/no-unused-vars 97:33 error 'vm' is defined but never used @typescript-eslint/no-unused-vars 97:37 error 'info' is defined but never used @typescript-eslint/no-unused-vars /drone/src/src/types/shims-vue.d.ts 12:10 error 'ComponentCustomProperties' is defined but never used @typescript-eslint/no-unused-vars /drone/src/src/views/tasks/ShowTasks.vue 94:11 error 'key' is defined but never used @typescript-eslint/no-unused-vars ✖ 7 problems (7 errors, 0 warnings) error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. ```
dpschen force-pushed feature/feat-improve-ts-setup from a26441206b to 772668e91b 2022-04-13 19:29:20 +00:00 Compare
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1812-feature-feat-improve-ts-setup--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1812-feature-feat-improve-ts-setup--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen changed title from WIP: feature/feat-improve-ts-setup to feature/feat-improve-ts-setup 2022-04-13 20:12:14 +00:00
Author
Member

I guess this is already much better.

@konrad: any idea how to disable ts from accessing the drone ts stuff:

+ yarn run lint
yarn run v1.22.18
$ eslint --ignore-pattern '*.test.*' ./src --ext .vue,.js,.ts

/drone/src/src/indexes/index.ts
  1:19  error  'SimpleDocumentSearchResultSetUnit' is defined but never used  @typescript-eslint/no-unused-vars

/drone/src/src/main.ts
  87:33  error  'vm' is defined but never used    @typescript-eslint/no-unused-vars
  87:37  error  'info' is defined but never used  @typescript-eslint/no-unused-vars
  97:33  error  'vm' is defined but never used    @typescript-eslint/no-unused-vars
  97:37  error  'info' is defined but never used  @typescript-eslint/no-unused-vars

/drone/src/src/types/shims-vue.d.ts
  12:10  error  'ComponentCustomProperties' is defined but never used  @typescript-eslint/no-unused-vars

/drone/src/src/views/tasks/ShowTasks.vue
  94:11  error  'key' is defined but never used  @typescript-eslint/no-unused-vars

✖ 7 problems (7 errors, 0 warnings)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Okay I was confused when I wrote this :D — fixed

> I guess this is already much better. > > @konrad: any idea how to disable ts from accessing the drone ts stuff: > > ``` > + yarn run lint > yarn run v1.22.18 > $ eslint --ignore-pattern '*.test.*' ./src --ext .vue,.js,.ts > > /drone/src/src/indexes/index.ts > 1:19 error 'SimpleDocumentSearchResultSetUnit' is defined but never used @typescript-eslint/no-unused-vars > > /drone/src/src/main.ts > 87:33 error 'vm' is defined but never used @typescript-eslint/no-unused-vars > 87:37 error 'info' is defined but never used @typescript-eslint/no-unused-vars > 97:33 error 'vm' is defined but never used @typescript-eslint/no-unused-vars > 97:37 error 'info' is defined but never used @typescript-eslint/no-unused-vars > > /drone/src/src/types/shims-vue.d.ts > 12:10 error 'ComponentCustomProperties' is defined but never used @typescript-eslint/no-unused-vars > > /drone/src/src/views/tasks/ShowTasks.vue > 94:11 error 'key' is defined but never used @typescript-eslint/no-unused-vars > > ✖ 7 problems (7 errors, 0 warnings) > > error Command failed with exit code 1. > info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. > ``` Okay I was confused when I wrote this :D — fixed
dpschen reviewed 2022-04-13 20:15:13 +00:00
@ -0,0 +10,4 @@
// "allowJs": true,
// "allowSyntheticDefaultImports": true,
// "sourceMap": true,
// "strictNullChecks": true,
Author
Member

I was not sure about these options.
The latter should probably be enabled.

I was not sure about these options. The latter should probably be enabled.

Does the linter still pass when it is enabled?

Does the linter still pass when it is enabled?
Author
Member

This only affects tslint.
I googled a bit and I think I would use the following:

    "importHelpers": true,
    "sourceMap": true,
    "strictNullChecks": true,
    // "allowJs": true,
    // "allowSyntheticDefaultImports": true,

If we need to we can still use js if we disable the rule for that line.
It seems good to have allowSyntheticDefaultImports disabled

This only affects tslint. I googled a bit and I think I would use the following: ``` "importHelpers": true, "sourceMap": true, "strictNullChecks": true, // "allowJs": true, // "allowSyntheticDefaultImports": true, ``` If we need to we can still use js if we disable the rule for that line. It seems good to have `allowSyntheticDefaultImports` disabled
Author
Member

Changed.
Anything else missing for the merge?

Changed. Anything else missing for the merge?

I think this should be fine.

I think this should be fine.
dpschen marked this conversation as resolved
konrad reviewed 2022-04-18 17:17:55 +00:00
@ -0,0 +6,4 @@
"composite": true,
"baseUrl": ".",
// "importHelpers": true,

Isn't this invalid json with the //?

Isn't this invalid json with the `//`?
Author
Member
Yes: - https://github.com/microsoft/TypeScript/issues/4987 - https://www.reddit.com/r/typescript/comments/8na5vb/tsconfigjson_isnt_strict_json_what_now/
dpschen marked this conversation as resolved
dpschen force-pushed feature/feat-improve-ts-setup from 7fdb13780d to f735a58c51 2022-04-23 11:20:53 +00:00 Compare
konrad was assigned by dpschen 2022-04-23 12:26:48 +00:00
Author
Member

@konrad Anything blocking this from beeing merged?

@konrad Anything blocking this from beeing merged?
dpschen force-pushed feature/feat-improve-ts-setup from 82d7e31f4a to d61ecec53a 2022-05-10 23:32:47 +00:00 Compare
dpschen force-pushed feature/feat-improve-ts-setup from d61ecec53a to 44fa1e3697 2022-05-20 19:30:03 +00:00 Compare
dpschen force-pushed feature/feat-improve-ts-setup from 44fa1e3697 to aca400ff52 2022-05-23 22:05:57 +00:00 Compare
dpschen requested review from konrad 2022-05-23 22:09:38 +00:00
konrad reviewed 2022-05-27 17:30:43 +00:00
konrad left a comment
Owner

Why did you rename src/types/window.d.ts to crowdin.cli?

Why did you rename `src/types/window.d.ts` to `crowdin.cli`?
konrad reviewed 2022-05-27 17:34:25 +00:00
@ -0,0 +20,4 @@
export default (on, config) => {
// `on` is used to hook into various events Cypress emits
// `config` is the resolved Cypress config
on('dev-server:start', (options) => {

Oh so this will start a dev server when starting cypress?

Oh so this will start a dev server when starting cypress?

I think we should document this in the cypress readme.

I think we should document this in the cypress readme.
Author
Member

I think it waits for the server to start.

I thought I got this from the create-vue template as well. But seems like it's not from there. Searching via sourcegraph it seems I got it from some of the cypress documentation.

I think it waits for the server to start. I thought I got this from the create-vue template as well. But seems like it's not from there. [Searching via sourcegraph](https://sourcegraph.com/search?q=context:global+dev-server:start&patternType=literal) it seems I got it from some of the cypress documentation.

I think it would be good to know what this actually does and document it properly...

I think it would be good to know what this actually does and document it properly...
Author
Member

Okay I think I get it now:

It's for the new component testing. We aren't doing this yet, so there is nothing to configure yet. This just enables us to do this in the future. If we do, we should document it. As of now I don't see the need, since it's just the preparation not an actual usage.

I do not grasp yet when cypress component testing makes sense for us, since if I understand it correctly goes a bit into the domain of what vitest does.

Okay I think I get it now: It's for the new component testing. We aren't doing this yet, so there is nothing to configure yet. This just enables us to do this in the future. If we do, we should document it. As of now I don't see the need, since it's just the preparation not an actual usage. I do not grasp yet when cypress component testing makes sense for us, since if I understand it correctly goes a bit into the domain of what vitest does.
Author
Member

Component testing in cypress 10 doesn't work via plugins anymore.
Plugins were removed, so I removed them as well.

See: ac1004f96e

and

https://docs.cypress.io/guides/references/migration-guide#Plugins-File-Removed=

Component testing in cypress 10 doesn't work via plugins anymore. Plugins were removed, so I removed them as well. See: https://kolaente.dev/vikunja/frontend/commit/ac1004f96e333070fad443e465ea9e17804f637b and https://docs.cypress.io/guides/references/migration-guide#Plugins-File-Removed=
dpschen marked this conversation as resolved
@ -0,0 +4,4 @@
"compilerOptions": {
"isolatedModules": false,
"target": "es5",
"lib": ["es5", "dom"],

Not es6?

Not `es6`?
Author
Member
I took this from here: https://github.com/vuejs/create-vue-templates/blob/main/typescript-router-vitest-cypress/cypress/tsconfig.json Not sure about the reason.

Does it work with es6?

Does it work with `es6`?
Author
Member

Tbh I don't even understand why the cypress folder needs its own tsconfig.
So I'm not really sure what implications the change to es6 would have.
Since there don't seem any problems in the test I guess keeping it as is seems fine.

Does that work for you?

Tbh I don't even understand why the cypress folder needs its own tsconfig. So I'm not really sure what implications the change to es6 would have. Since there don't seem any problems in the test I guess keeping it as is seems fine. Does that work for you?

Ah, didn't see this was the cypress tsconfig.

I think this is fine.

Ah, didn't see this was the cypress tsconfig. I think this is fine.
konrad marked this conversation as resolved
Author
Member

Why did you rename src/types/window.d.ts to crowdin.cli?

I didn't. They are both empty files. That's why git though I have moved them.
Not sure where the file is coming from though. I guess can be removed if there is'nt something useful that we want to add.

> Why did you rename `src/types/window.d.ts` to `crowdin.cli`? I didn't. They are both empty files. That's why git though I have moved them. Not sure where the file is coming from though. I guess can be removed if there is'nt something useful that we want to add.
Author
Member

By the way: Cypress 10 is coming. Not sure what that means for us…

By the way: [Cypress 10 is coming](https://twitter.com/_jessicasachs/status/1529567980434165761). Not sure what that means for us…
Owner

Since cypress 10 is now merged, could you rebase on main and check again?

Other than my two last comments this should be good to merge.

Since cypress 10 is now merged, could you rebase on `main` and check again? Other than my two last comments this should be good to merge.
dpschen force-pushed feature/feat-improve-ts-setup from aca400ff52 to 000ba4a502 2022-06-21 20:30:37 +00:00 Compare
dpschen force-pushed feature/feat-improve-ts-setup from 000ba4a502 to d78be8f859 2022-06-21 21:10:33 +00:00 Compare
dpschen force-pushed feature/feat-improve-ts-setup from ac1004f96e to cedada85ea 2022-06-21 22:23:46 +00:00 Compare
konrad approved these changes 2022-07-04 15:44:58 +00:00
konrad left a comment
Owner

Should be fine, please feel free to merge once rebased on main.

Thank you for the effort.

Should be fine, please feel free to merge once rebased on `main`. Thank you for the effort.
dpschen force-pushed feature/feat-improve-ts-setup from cedada85ea to 345cb07605 2022-07-04 21:39:57 +00:00 Compare
dpschen merged commit c6d214b9eb into main 2022-07-04 21:50:50 +00:00
dpschen deleted branch feature/feat-improve-ts-setup 2022-07-04 21:50:50 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.