full-stack-fastapi-template icon indicating copy to clipboard operation
full-stack-fastapi-template copied to clipboard

:recycle: Refactor frontend

Open cbhagl opened this issue 4 years ago • 13 comments

Hi,

thanks for the great work on FastAPI and this template. We are using it in my team at Hella Aglaia Mobile Vision GmbH to build an internal tool for our machine learning services. While working with the frontend template, I have noticed that some of the dependencies are outdated, so I started refactoring the frontend to bring everything up-to-date. Here's what I did.

  • :recycle: Migrate to latest Vuetify 2.x API, for details see migration guide (https://vuetifyjs.com/en/getting-started/releases-and-migrations/#migration-guide)
  • :recycle: Migrate to latest Vee-Validate 3.x API, for details see migration guide (https://logaretm.github.io/vee-validate/migration.html#migration-guide)
  • :heavy_plus_sign: Refactor state management with vuex-module-decorators (https://github.com/championswimmer/vuex-module-decorators), which is similar in style to vue-class-components
  • :sparkles: Add configuration for eslint (+prettier, +typescript)
  • :art: Reformat code with eslint --fix
  • :pushpin: Upgrade/add relevant dependencies
  • :truck: Remove external dependencies (e.g. mdi fonts) by adding them as direct dependencies

Maybe there is interest in these changes.

Cheers

cbhagl avatar Apr 22 '20 15:04 cbhagl

I'm still learning frontend development at the moment, could you or someone else explain why the test is failing?

If we switch to eslint, do we still need tslint.json?

When I do npm run lint I get errors like these in shims-tsx.d.ts although I don't have any issues when running npm run serve:

error: 'Vue' is defined but never used. Allowed unused vars must match /^_/u (@typescript-eslint/no-unused-vars) at src/shims-tsx.d.ts:1:8:
> 1 | import Vue, { VNode } from "vue";
    |        ^
  2 | 
  3 | declare global {
  4 |   namespace JSX {


error: 'VNode' is defined but never used. Allowed unused vars must match /^_/u (@typescript-eslint/no-unused-vars) at src/shims-tsx.d.ts:1:15:
> 1 | import Vue, { VNode } from "vue";
    |               ^
  2 | 
  3 | declare global {
  4 |   namespace JSX {


warning: Interface name must be prefixed with "I" (@typescript-eslint/interface-name-prefix) at src/shims-tsx.d.ts:6:15:
  4 |   namespace JSX {
  5 |     // tslint:disable no-empty-interface
> 6 |     interface Element extends VNode {}
    |               ^
  7 |     // tslint:disable no-empty-interface
  8 |     interface ElementClass extends Vue {}
  9 |     interface IntrinsicElements {

Should this be happening?

Nonameentered avatar May 02 '20 18:05 Nonameentered

Hi @Nonameentered,

thank you very much for pointing out these issues! I too have just started front-end development, so I really appreciate someone peer-reviewing my changes.

If we switch to eslint, do we still need tslint.json?

You are right, tslint.json is obsolete if we switch to eslint. I missed that!

When I do npm run lint I get errors like these in shims-tsx.d.ts although I don't have any issues when running npm run serve:

[...]

Should this be happening?

These shims are auto-generated by vue-cli and I think they can safely be ignored by eslint. I will add the corresponding configuration.

Regarding the changes you suggested, they all look valid. I will incorporate them into the pull request :)

Thanks again!

cbhagl avatar May 03 '20 11:05 cbhagl

Not sure how to/if it's possible to suggest these changes since it wasn't already edited in the PR.

I think one way could be to do a Pull Request on the branch inside my fork, but not sure.

cbhagl avatar May 03 '20 11:05 cbhagl

Looks like the Manage Users page is broken in that you can't click to edit the user through the actions column.

Nonameentered avatar May 04 '20 06:05 Nonameentered

Ah yes, I've been meaning to address this issue, but hadn't found time to do so yet.

Naming conventions in JavaScript mostly require camelCase variables, whereas in Python we like snake_case. This will of course trigger a linter on either side, depending which style you choose. In this case, we choose snake_case which will satisfy flake8 (with plugin pep8-naming) but trigger eslint (as configured in this pull request). However, there is a workaround for this, which will satisfy both: Pydantic models can be configured with an alias_generator that takes a function which converts all field names (e.g. snake to camel case) which can be used during serialization (model.json(by_alias=True)). See an example implementation of this here: https://github.com/nsidnev/fastapi-realworld-example-app/blob/master/app/models/domain/rwmodel.py This would make all schemas compatible with PEP8 in the backend and eslint in the frontend.

For now though, your changes are correct.

cbhagl avatar May 07 '20 09:05 cbhagl

Hi @tiangolo, any plan to merge this pr? Or should we add it to our fork?

monatis avatar Aug 07 '20 09:08 monatis

I forked @cbhagl's refactor-frontend branch and then pulled @tiangolo's master and pushed it into my fork. Anyone who wants to start with Vuetify 2 can use this one until this PR is merged.

monatis avatar Aug 14 '20 00:08 monatis

@Nonameentered why did you used ~ for some dependencies? Isn't ^ better, as this would pull new features without breaking anything? Or am I wrong here?

Koschi13 avatar Aug 28 '20 14:08 Koschi13

I think vue-cli deliberately uses ~ for the plugins it manages.

cbhagl avatar Aug 28 '20 14:08 cbhagl

Ok, maybe I mentioned the wrong user ^^ Anyway I got the right person, why don't we replace the ~ with ^. I don't know JS that much but it seems to be better to use ^ for the dependencies as we could only benefit from the new minor versions. Also this project serves as a template, therefore it would be more future proof to use ^ instead of ~.

Koschi13 avatar Aug 28 '20 15:08 Koschi13

I think vue-cli wants you to use vue upgrade to upgrade its plugins because those upgrades might involve some changes that need the users attention.

The safest approach I think is just to upgrade vue-cli every once in a while and update the template.

cbhagl avatar Aug 28 '20 16:08 cbhagl

@cbhagl didn't know that, thanks for the clarification.

Koschi13 avatar Aug 28 '20 16:08 Koschi13

I refactored my project by hand using your code here as a guide and it worked splendidly.

Excellent work! Thank you.

KevinGrey-sys avatar Jan 10 '21 22:01 KevinGrey-sys

Thanks for all the work here @cbhagl! :bow:

I wanted to refactor the frontend and move to React with TypeScript and hooks, etc. It's now been done in other PRs, so I'll pass on this one. But thanks for all the effort! :cake:

tiangolo avatar Mar 08 '24 10:03 tiangolo