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: add typescript support using typescript-build #328

Merged
merged 50 commits into from
Mar 17, 2020
Merged

feat: add typescript support using typescript-build #328

merged 50 commits into from
Mar 17, 2020

Conversation

ChangJoo-Park
Copy link
Contributor

@ChangJoo-Park ChangJoo-Park commented Aug 21, 2019

I do not add any other typescript dependencies like vue-property-decorator and vue-function-api . It may depends on users.
and hard to decide add tsconfig.json by default. please give me a suggestions.

image

image

@ChangJoo-Park ChangJoo-Park changed the title typescript support using typescript-build feat: typescript support using typescript-build Aug 21, 2019
@ChangJoo-Park
Copy link
Contributor Author

image

I don't know why circleci failed when testing with ava?

Copy link
Contributor

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Thanks @ChangJoo-Park for working on this !

I let a little comment about package.json.
Aside that, I think user should have the choice to use Runtime support, and the default layouts/pages/components should be converted in TypeScript (but i guess it's hard stuff doing it for every case depending of selected options)

template/_package.json Outdated Show resolved Hide resolved
@ChangJoo-Park
Copy link
Contributor Author

Thanks @ChangJoo-Park for working on this !

I let a little comment about package.json.
Aside that, I think user should have the choice to use Runtime support, and the default layouts/pages/components should be converted in TypeScript (but i guess it's hard stuff doing it for every case depending of selected options)

I think so, Runtime support is important.
when I create this PR, I did not use typescript-runtime and I didn't care about it.
also I will try to change components using TypeScript.
Thanks @kevinmarrec

@ChangJoo-Park
Copy link
Contributor Author

runtime-ts-default
runtime-ts

Can choose @nuxt/typescript-runtime when recommended none server + typescript selected

Copy link
Contributor

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Almost perfect :)

template/nuxt/pages/index.vue Show resolved Hide resolved
template/_package.json Outdated Show resolved Hide resolved
@ChangJoo-Park
Copy link
Contributor Author

all requests are done.

  • default language : js
  • show jsconfig.json only when select js
  • tsconfig.json always added when choose ts
  • show typescript nuxt cookbook links when choose ts
  • update <script lang="ts"> in index.vue` for ts

please review @kevinmarrec 🙇

template/nuxt/pages/index.vue Outdated Show resolved Hide resolved
@kevinmarrec
Copy link
Contributor

kevinmarrec commented Aug 30, 2019

@ChangJoo-Park Could you also take into account https://typescript.nuxtjs.org/guide/lint.html ?
You mostly need to

  1. Change https://github.com/nuxt/create-nuxt-app/blob/master/template/_package.json#L122 for eslint-config-typescript.
  2. Change https://github.com/nuxt/create-nuxt-app/blob/master/template/_.eslintrc.js#L16 by @nuxtjs/eslint-config-typescript
  3. Remove https://github.com/nuxt/create-nuxt-app/blob/master/template/_.eslintrc.js#L12 parserOptions (@nuxtjs/eslint-config-typescript already set it to a TypeScript parser)

I also think that for Nuxt TypeScript projects https://github.com/nuxt/create-nuxt-app/blob/master/template/_package.json#L123 (@nuxtjs/eslint-module) should be removed, I need to make typescript-build module enables eslint option of fork-ts-checker-webpack-plugin true by default.

@ChangJoo-Park
Copy link
Contributor Author

@ChangJoo-Park Could you also take into account https://typescript.nuxtjs.org/guide/lint.html ?
You mostly need to

  1. Change https://github.com/nuxt/create-nuxt-app/blob/master/template/_package.json#L122 for eslint-config-typescript.
  2. Change https://github.com/nuxt/create-nuxt-app/blob/master/template/_.eslintrc.js#L16 by @nuxtjs/eslint-config-typescript
  3. Remove https://github.com/nuxt/create-nuxt-app/blob/master/template/_.eslintrc.js#L12 parserOptions (@nuxtjs/eslint-config-typescript already set it to a TypeScript parser)

I also think that for Nuxt TypeScript projects https://github.com/nuxt/create-nuxt-app/blob/master/template/_package.json#L123 (@nuxtjs/eslint-module) should be removed, I need to make typescript-build module enables eslint option of fork-ts-checker-webpack-plugin true by default.

1,2. Changed @nuxtjs/eslint-config-typescript from @nuxtjs/eslint-config and @nuxtjs/eslint-module.
3. remove parserOptions when using TypeScript

  • Set tsRuntime false when select JavaScript

@ChangJoo-Park
Copy link
Contributor Author

I'll check this :)

@ChangJoo-Park
Copy link
Contributor Author

@kevinmarrec Can you try lint on ubuntu? I tried my macbook (MacOS Catalina, Node.js v10), lint works well. npm run lint too.
😭

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Jan 24, 2020

@ChangJoo-Park

Failing on my Ubuntu 19.04 :
image

It's cause before there wasn't template conditional check in template/frameworks/jest/jest.config.js

Can you confirm that the template correctly has the right content with or without TypeScript when generating new project with your changes ?

We can eventually add an entry in .eslintignore to prevent eslint to lint-check it : https://github.com/nuxt/create-nuxt-app/blob/master/.eslintignore

There are already 2 Vue files prevented to be linted cause there are using a template pattern <%= %>
https://github.com/nuxt/create-nuxt-app/blob/master/template/nuxt/pages/index.vue#L6
https://github.com/nuxt/create-nuxt-app/blob/master/template/frameworks/iview/pages/index.vue#L6

Adding template/frameworks/jest/jest.config.js fixes the lint issue.

@vinayakkulkarni
Copy link
Contributor

We also have to test TS integration against all UI & testing frameworks right?

I can help out with Ava

@NickBolles
Copy link

@vinayakkulkarni See my PR here: https://github.com/ChangJoo-Park/create-nuxt-app/pull/1

That covers a big number of them. After this gets merged I can submit a followup from that branch

@kevinmarrec
Copy link
Contributor

@ChangJoo-Park Can you fix the tests by following my last instructions ? Thanks !

@ChangJoo-Park
Copy link
Contributor Author

I try to fix out this lint failing, I didn't 😭

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Feb 4, 2020

@ChangJoo-Park Seems the lint is fixed, but that's the tests that are failing now (the test suite wasn't previously processed cause of the lintstep resulting in an error)

Some of them are passing, seems the others have been broken by some change ?

@kevinmarrec
Copy link
Contributor

@ChangJoo-Park Could you fix the conflicts ? :)

@ChangJoo-Park
Copy link
Contributor Author

Sure I'll fix it

template/_package.json Outdated Show resolved Hide resolved
template/_package.json Outdated Show resolved Hide resolved
template/_package.json Outdated Show resolved Hide resolved
@atinux atinux merged commit e4b9cd8 into nuxt:master Mar 17, 2020
@ChangJoo-Park
Copy link
Contributor Author

Thanks @atinux :)

@vinayakkulkarni
Copy link
Contributor

Whooo!

@murata-kawairi-business

Thanks! for Contributors

@davelsan
Copy link

Thank you for this PR, I was awaiting it with expectation. I have created the first app and it resembles very closely what I had already been using for myself.

I've made a few suggestions in #448, including fixes to some problems that might arise after creating a new app. I'm leaving this comment here for anybody that might experience issues, in case it can help.

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.