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/Feat] Improvements to recently merged typescript-build support #448

Closed
davelsan opened this issue Mar 18, 2020 · 4 comments
Closed

Comments

@davelsan
Copy link

davelsan commented Mar 18, 2020

I'd like to raise an issue here, for future reference, about a few improvements that I think could be made after the recently merged PR #328.

I have created a new app with the below typescript support options and these are the things I found that (IMHO) can be fixed or improved.

create-nuxt-app v2.15.0
✨  Generating Nuxt.js project in nuxt-test
? Project name nuxt-test
? Project description My brilliant Nuxt.js project
? Author name David Velasco
? Choose programming language TypeScript
? Choose the package manager Yarn
? Choose UI framework Tailwind CSS
? Choose custom server framework None (Recommended)
? Choose the runtime for TypeScript @nuxt/typescript-runtime
? Choose Nuxt.js modules Progressive Web App (PWA) Support, DotEnv
? Choose linting tools ESLint
? Choose test framework Jest
? Choose rendering mode Single Page App
? Choose development tools jsconfig.json

Fixes

[Fix] File XXX is not a module (Vetur error)

Update: There is now a PR vuejs/vetur#1806 addressing this issue.

This has been reported in vuejs/vetur#1187 and vuejs/vetur#1709. Any component that does not export a default vue instance within the script tags, causes the following error:

File XXX is not a module (Vetur 2306)

pages/index.vue

<script lang="ts">
    import Vue from 'vue';
    import Logo from '~/components/Logo.vue'; // <- error here

    export default Vue.extend({
      components: {
        Logo
      }
    });
</script>

For now the workaround is to export an empty instance.

components/Logo.vue

<script lang="ts">
  import Vue from 'vue';
  export default Vue.extend({

  });
</script>

[Fix] Remove linting of unused vars

In nuxt.config.js, eslint will output the following errors for config and ctx in the extend webpack config section:

'config' is defined but never used. Allowed unused args must match /^/u.
'ctx' is defined but never used. Allowed unused args must match /^
/u.

// nuxt.config.js
module.exports = {
  // ...
  build: {
    /*
    ** You can extend webpack config here
    */
    extend (config, ctx) { } // <- error here
  }
}

To fix it, we could either remove the empty method, add them to the ignore pattern or switch off the @typescript-eslint/no-unused-vars rule. For example:

{
  // .eslintrc.json
  // ...
  "rules": {
    "@typescript-eslint/no-unused-vars": [ "error", {
      "argsIgnorePattern": "config|ctx"
    }],
  }
}

[Fix] Do not use the base ESLint indent rule

This has already been reported in nuxt/eslint#76. Using the base eslint indent rule causes, among other inconsistencies, the following error

Cannot read property 'loc' of undefined

My initial fix was to switch off the base indent rule and use vue/script-indent instead.

{
  // .eslintrc.json
  // ...
  "rules": {
    
    "indent": "off",

    "vue/script-indent": ["warn", 2, { 
      "baseIndent": 2
    }]
  }
}

Note above that something is off about the default vue/script-indent, possibly something nuxt is configuring by default under the hood. The following settings add a base indentation of 4 spaces, when it should be 2. That is, the code indentation is being applied to base as well.

However, for this to work in *.ts files and not conflict with vue rules, I believe the proper configuration could be to use @typescript-eslint/no-indent and vue/script-indent override rules, for *.ts and *.vue files, respectively.

{
  // .eslintrc.json
  // ...
  // A single override might work, but it's possible that two overrides need to be set up,
  // one for *.vue and another for *.ts files, with specific rules for each.
  "overrides": [
    {
      "files": [ "*.ts" ],
      "rules": {
        "@typescript-eslint/indent": [ "warn", 2 ]
      }
    }
  ]

Features

[Feat] Add TypeScript linting to package.json scripts

Not really an issue (though it might be in CI), since we are all used to have editor linting support. However, to lint *.ts files from the console, I believe the lint script in package.json needs to be aware of the extension.

{
  // package.json
  // ...
  "scripts": {
    "dev": "nuxt-ts",
    "build": "nuxt-ts build",
    "start": "nuxt-ts start",
    "generate": "nuxt-ts generate --spa",
    "lint": "eslint --ext .ts,.js,.vue --ignore-path .gitignore .",  // <- added .ts
    "test": "jest"
  },
 // ...
}

[Feat] Add TypeScript support to Jest test files

In jest.config.ts there is support for *.ts transforms, but *.spec.ts files are not supported unless @types/jest is installed and the vue module is declared, for example in types/vue.shims.d.ts

yarn add @types/jest -D
// vue.shims.d.ts
declare module '*.vue' {
  import Vue from 'vue';
  export default Vue;
}

[Feat] Port Nuxt config to TypeScript

Nothing wrong here, but I like to have config files written in *.ts, both for consistency and tooling support. For example, to have intellisense in editors such as vscode.

// nuxt.config.ts
import { Configuration } from '@nuxt/types';

const config: Configuration = {
  // same config options
};

export default config;

Note that if @typescript-eslint/no-unused-vars is switched off, for this to work without linting errors the base eslint rule no-unused-vars must be switched off as well. This is recommended in the official docs, too.

Edits

  • [fix] the vetur error has a PR incoming
  • [feat] *.ts file linting in lint script within package.json
  • [fix] pages/index.vue and components/Logo.vue examples
  • [all] typos and style fixes
@NickBolles
Copy link

NickBolles commented Mar 28, 2020

Nice. I had done some of this in my PR to the original dev before merging here: https://github.com/ChangJoo-Park/create-nuxt-app/pull/1
and now here
#449

I'll look at the differences, maybe I can leave the component updates that I've done and this PR can handle the more core stuff This is an issue, not a PR... oops.

@davelsan
Copy link
Author

Yes, it spreads a bit too much for a single issue, but I thought some points needed a bit of discussion before considering a PR. For example:

  • The vetur error should probably be handled in their repository, so the suggested fix is more likely just a workaround.
  • The proposed changes to eslint indentation rules need a bit of attention too, especially the reason why script-indent is being added to base-indent (could be a config issue)

That said, I somehow missed your PR and I see that some points are already addressed there. Maybe some of the other proposals could be included in #449? I'd be happy to move the discussion there and close this. I'll check it again Monday, see what else you got planned.

@NickBolles
Copy link

The vetur error should probably be handled in their repository, so the suggested fix is more likely just a workaround.
I don't think this is possible. Plus the official recomendation for @nuxtjs/typescript is to create the vue-shims.d.ts documentation. This is included in my PR.

I'm working on the other parts of the PR right now. See the latest comment for a summary of your issues and the fixes for them.

@davelsan
Copy link
Author

I'm working on the other parts of the PR right now. See the latest comment for a summary of your issues and the fixes for them.

Good stuff! I'm closing this issue in favor of #449. We can continue the discussion there.

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

No branches or pull requests

2 participants