Skip to content
This repository has been archived by the owner on Nov 8, 2021. It is now read-only.

Migration from TSLint to ESLint #22

Merged
merged 2 commits into from
Jul 21, 2019
Merged

Migration from TSLint to ESLint #22

merged 2 commits into from
Jul 21, 2019

Conversation

IlCallo
Copy link
Member

@IlCallo IlCallo commented Jul 19, 2019

Some things I don't really know how to do, so I'm listening for your suggestions:

  • I took as assumptiom that ESLint dependencies was installed, but I guess it's not always the case. Should I use hasPackage for all packages needed by the extension but that could already be there from the initial project creation if ESLint option had been selected?
  • I wanted to add some needed configuration for Vetur and ESLint plugin for VSCode but didn't know how to detect if the user was using it or not (and how would I, after all?). For now I just put a new question
  • There is an old TODO wondering how to detect if yarn or npm was used in first place. Maybe we can try to infer it from yarn.lock or package.lock presence?
  • Some files need particular typings for their parameters, I ust left them untyped for now... Read more below on this;
  • Check linting rules and Prettier configuration, if you prefer a different standard (or leave Prettier out of this altogether)

I think TS support (and its linting) should be done in the main repo, because linting is too much coupled with initial project choices to be done in an extension.

An idea, on the long run, could be to write all starter kit files as TS files, and transpile them to normal JS when creating the project for someone who doesn't want TS.
It's much easier to downgrade TS files before, than to augment typings for all possible generated files afterwards.

PS: I refactored the install.js file using async/await because I was having a really hard time understanding what was going on there, in the nested callback hell. I hope it's more readable like this

@outofmemoryagain
Copy link
Collaborator

outofmemoryagain commented Jul 19, 2019

Overall it looks really good. Here is my feedback so far:

  • I'm not sure about the package detection from an AE, @nothingismagick, @nklayman any thoughts?
  • For vscode configuration prompting the user if perfect, but instead of assuming that you want to use all the options you have added, we should include a follow up prompt to select the list of desired features (prettier, templateInterpolationService, autofix lint issues ). This is similar to how the Quasar starter lists optional additions that can be added to the project when created (axios, i18n, vuex...). For example, not everyone chooses to use prettier so at least give them an options to opt out, most people will probably choose if it is preselected. We should also update the docs if any follow up is needed (extension install for example).
    • Remove inclusion of gitlens extension. Although useful it is not related to quasar or typescript configuration. This would be better kept in a "Recommended Tools" section of the documentation.
  • This is how the Quasar CLI determines if it should use Yarn, the extension should probably do the same.
  • I mentioned Prettier above, but as far as the linting ruleset goes, I think we should prompt for that as well. In tslint I prefer to use tslint:recommended ruleset, which differ from what you currently have configured (especially quotes, and semis), so we should make sure we handle that, or at the very least document how to adjust the settings to meet the needs of the project.

In general, I do agree with your thoughts on the Starter vs Extension. Especially when it comes to things like bootfiles. Anytime we modify code in an existing file we are taking a risk that the user doesn't have something special that we cause to break.

@outofmemoryagain
Copy link
Collaborator

On a related it note it might be a good idea to detect the axios boot file exists and include the shim for axios (addressing your issue quasarframework/quasar#4479). If we don't have confidence in scanning the project, we should probably prompt the user to ask them "Add Axios type definitions" or something along those lines...

@outofmemoryagain
Copy link
Collaborator

I just ran some tests linking the branch extension to it to a test project. I found that I had to manually add "ts-loader" as a devDependency directly to the project's package.json file otherwise it could not build correctly.

@outofmemoryagain
Copy link
Collaborator

@IlCallo I went ahead and made the suggested changes. It's late so I will sleep on them a bit since I'm not entirely sure about how I broke out the prettier prompt option and the associated configuration.
Mostly because of the timing of when templates are actually written and it essentially prevent dynamic modifications to them without embedding the entire template in code and writing it out directly instead of the using the api.render feature. I will probably submit the changes and accept the PR tomorrow so we can try to get it published tomorrow. I would like to get it updated as soon as possible, as I noticed a new issue that would prevent linting from working properly in the currently published extension.

…rly installed in the project.

Add an additional prompt for Prettier.
@outofmemoryagain outofmemoryagain merged commit d12a023 into quasarframework:dev Jul 21, 2019
// Required to lint *.vue files
// See https://eslint.vuejs.org/user-guide/#why-doesn-t-it-work-on-vue-file
'vue'
// Prettier has not been included as plugin to avoid performance impact
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be removed in the "no-prettier" version

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, let's get a new PR going then. If we need a new issue to capture the issues we should do so.

@IlCallo
Copy link
Member Author

IlCallo commented Jul 22, 2019

Remove inclusion of gitlens extension

Right, shouldn't be there in the first place

This is how the Quasar CLI determines if it should use Yarn, the extension should probably do the same.

Wonderful, didn't saw that. Do we have access to that utility function into AE installation file?

I found that I had to manually add "ts-loader" as a devDependency directly to the project's package.json file otherwise it could not build correctly.

Then it should probably be added with api.extendPackageJson() like the other critical ones, instead of moving all of them into dependencies again.
It's not entirely clear how different type of dependencies work with the AE system...
Also, it seems to be a previous problem? Look at #20

@IlCallo IlCallo deleted the typescript-eslint branch July 22, 2019 08:30
@razbakov
Copy link

Does it address #18?

@IlCallo
Copy link
Member Author

IlCallo commented Jul 22, 2019

Tecnically yes, but probably needs some more work to really resolve all the stuff written there, a new PR should be done.
It has been merged quickly because of this:

I will probably submit the changes and accept the PR tomorrow so we can try to get it published tomorrow. I would like to get it updated as soon as possible, as I noticed a new issue that would prevent linting from working properly in the currently published extension.

@outofmemoryagain
Copy link
Collaborator

outofmemoryagain commented Jul 22, 2019

@IlCallo I think we need further discussion with the AE guys on the dependencies topic. We could use extendPackageJson to add it directly to the project's package.json file, but I'm not sure if that is the intention or not. Based on other extensions, dependencies are generally pulled through based on the extensions dependencies. I had a bit of a discussion over the weekend, and this seems to be the current approach. I would however like more clarity on it myself. But I agree the current way of adding "ts-loader" need to be changed.

This is how the Quasar CLI determines if it should use Yarn, the extension should probably do the same.

Wonderful, didn't saw that. Do we have access to that utility function into AE installation file?

No I don't think that is exposed. It would be good to see it added, than and deepmerge for json objects would be nice additions.

@nothingismagick
Copy link
Contributor

I am kind of out on a limb here about deps. Our strategy (and indeed the status quo) with AE's is to inject dependencies via the AE package.json - because this is something we can control. Once its in your project package.json - we can't easily (nor should we really) touch that.

Although extendPackageJson will trigger a .__needsNodeModulesUpdate if the deps are touched, just because this feature is available doesn't necessarily mean we should or shouldn't be using it in official AE's. Hence me being on a limb and unsure which side of the branch I should cut off...

That said, there are subtle inconsistencies across core though (e.g. when adding the electron mode, we DO add packages with yarn etc.) When Razvan gets back the entire dependency system across the Quasar Ecosystem will be revisited, and I'll be sure to address this.

Until then: status quo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants