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

Feat: Generate typescript package types and tests #449

Closed
wants to merge 60 commits into from

Conversation

NickBolles
Copy link

@NickBolles NickBolles commented Mar 20, 2020

Resubmitting as a pr to the base repo.

Original PR: https://github.com/ChangJoo-Park/create-nuxt-app/pull/1
But it didn't make it into the initial typescript support here: #449

List of changes

  1. Create nuxt.config.ts

  2. Update nuxt.config.ts with changes from nuxt.config.js

  3. Create a generate component template helper to switch between javascript and typescript

    • Another approach, if there are more typscript differences, could be a seperate file for js and typescript. but that seemed unnecessary right now
  4. Update the following components to generate typescript components using the generateComponent template helper

    • buefy
    • FrameVuerk
    • Iview
    • Vuesax
    • Vuetify
  5. Add typescript files to eslint and lint-staged configs

  6. fix tests for typescript runtime

  7. Add vue-shims.d.ts file

Issues from #448 - @davelsan

  1. [Fix] File XXX is not a module (Vetur error)
  • vue-shims.d.ts included above
  1. [Fix] Remove linting of unused vars
  • Add a fix by prefixing them with a _, which is the standard way to declare an unused var
  1. [Fix] Do not use the base ESLint indent rule
  • No fix for this in this PR. I'm going to leave this for another issue
  1. [Feat] Add TypeScript linting to package.json scripts
  • Included above
  1. [Feat] Add TypeScript support to Jest test files
  • added @types/jest as suggested by @davelsan
  1. [Feat] Port Nuxt config to TypeScript
  • Included above

@mtltechtemp
Copy link

Is this important enough to patch the package? Seems like it might belong in a seperate pr?

That's just a 3 line patch to make the ejs lib do the work.

@mtltechtemp
Copy link

mtltechtemp commented Apr 7, 2020

@NickBolles Did you understand why we have error clear vue/comment-directive ?

Edit:
Apparently "view/comment-directive" is used to disable eslint rules by making comments in the template.
There is a bug with this when this directive is enabled.
So I simply disabled it:

{
  "plugins": ["ejs"],
  "extends": ["@nuxtjs"],
  "globals": {
    "use": true
  },
  "rules": {
    "no-console": "off",
    "vue/comment-directive": "off"
  }
}

At least it's not as bad as having no linter at all.

@NickBolles What do you think about that?

@mtltechtemp
Copy link

@NickBolles ping

@NickBolles
Copy link
Author

@mtltechtemp If you want to put a pull request together, either to follow up this one, or to merge into this branch I'd be fine with that. Although IMO a better fix would be to update eslint-plugin-ejs to register a pre-processor ID (https://eslint.org/docs/developer-guide/working-with-plugins#processors-in-plugins) and then allow users to configure it via the processor config, or overrides configs: https://eslint.org/docs/user-guide/configuring#specifying-processor

I hate doing one time patches of modules when with a little bit more work everyone can benefit.

@@ -61,6 +61,15 @@
</div>
</template>

<%- generateComponent(`
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like the script section hasn't been removed from this file

@@ -86,6 +86,7 @@ module.exports = {
/*
** Nuxt.js dev-modules
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this carriage return is needed as it's not done for the other comments on top of properties.

Suggested change

/*
** Nuxt.js dev-modules
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this carriage return is needed as it's not done for the other comments on top of properties.

Suggested change

@@ -0,0 +1,182 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Useless carriage return at beginning of file

Suggested change

@@ -25,7 +25,9 @@
},
"types": [
"@types/node",
"@nuxt/types"
"@nuxt/types"<%_ if (ui === 'vuetify') { _%>,
"vuetify"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"vuetify"
"@nuxtjs/vuetify"

Comment on lines 33 to 40
let configFile
if (answers.server === 'adonis') {
configFile = 'config/nuxt.js'
} else if (typescript) {
configFile = 'nuxt.config.ts'
} else {
configFile = 'nuxt.config.js'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let configFile
if (answers.server === 'adonis') {
configFile = 'config/nuxt.js'
} else if (typescript) {
configFile = 'nuxt.config.ts'
} else {
configFile = 'nuxt.config.js'
}
const configFile = answers.server === 'adonis' ? 'config/nuxt.js' : `nuxt.config.${typescript ? 'ts' : 'js'}`

saofile.js Outdated
Comment on lines 12 to 13
const tsRuntime =
this.answers.runtime && this.answers.runtime.includes('ts-runtime')
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO Unnecessary change

saofile.js Outdated
return `
<script ${ifTrue(typescript, 'lang="ts"')}>
${ifTrue(typescript, 'import Vue from \'vue\'')}
${ifTrue(!!imports, imports)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is there are imports, a carriage return should be ensured between imports and export.

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Apr 12, 2020

@NickBolles Could you take into account my requested changes ? then make your PR compatible with new monorepo (that's why you get lot of conflicts)

@kevinmarrec
Copy link
Contributor

@NickBolles Can you fix conflicts & tests ? Thanks :)

@NickBolles
Copy link
Author

NickBolles commented Apr 23, 2020

@kevinmarrec updated. Could you take a look at this today? It's getting kinda difficult to keep this up to date with all of the other changes going on. :/

Edit: also looks like there are some timeout requests on the CI checks

@kaangokdemir
Copy link

kaangokdemir commented Jun 6, 2020

Great feature and nice work @NickBolles

@Atinux
Copy link
Member

Atinux commented Jul 22, 2020

@NickBolles fo you mind resolving the conflicts one last time?

@ZachJW34 ZachJW34 mentioned this pull request Sep 8, 2020
3 tasks
@scscgit
Copy link
Contributor

scscgit commented Apr 10, 2022

Is there any plan to continue working on this, or will it be changed after Nuxt 3 anyway?

@NickBolles NickBolles closed this Jul 25, 2023
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

Successfully merging this pull request may close these issues.

None yet

9 participants