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

JSDoc type fixes in src/main/core-options.js #6702

Merged
merged 1 commit into from Oct 25, 2019

Conversation

brodybits
Copy link
Contributor

with unused oppositeDescription parameter removed

and reordering of some other members

to resolve checkJs issues in src/main/core-options.js,

as verified by the following command:

npx tsc --allowJs --checkJs --noEmit --target es5 src/main/core-options.js

I can also enable Check JS in VSCode and see that the type errors go away in src/main/core-options.js.

In the future, it should be possible to typecheck the whole project by either:

  • using a command like this (99% solution): npx tsc --allowJs --checkJs --noEmit --resolveJsonModule --target es5 src/index.js
  • add tsconfig.json and just do npx tsc or use a script that calls tsc to check all sources

for reference:

see also:

/cc @Shinigami92 @rbiggs

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

with unused `oppositeDescription` parameter removed

and reordering of some other members

to resolve checkJs issues in `src/main/core-options.js`,

as verified by the following command:

    npx tsc --allowJs --checkJs --noEmit --target es5 src/main/core-options.js
@lydell
Copy link
Member

lydell commented Oct 24, 2019

This looks good to me!

Adding a tsconfig.json won’t hurt, will it? Sounds like a good idea to me, since it would make things easier to check in the future.

@brodybits
Copy link
Contributor Author

Adding a tsconfig.json won’t hurt, will it?

No but we may want to coordinate it since @Shinigami92 already proposed a tsconfig.json file in #6313.

I made this change to be self-contained on purpose, so we should be able to merge it as-is if accepted.

I do have some other changes for JSDoc type fixes in my workarea which may need some better coordination. Maybe I should raise an issue to coordinate these.

Thanks!

This was referenced Oct 24, 2019
@rbiggs
Copy link

rbiggs commented Oct 24, 2019

I don't believe you would need a tsconfig file in this case sense you don't have any actual TypeScript that your compiling. What you could use is a jsconfig.json file, which would help with the checkjs test for your JavaScript files.

@thorn0
Copy link
Member

thorn0 commented Oct 24, 2019

@rbiggs Is there any practical difference between jsconfig.json and tsconfig.json?

upd: My testing shows that tsc ignores jsconfig.json.

@brodybits
Copy link
Contributor Author

This is going off-topic, but I think we need tsconfig.json and not jsconfig.json to configure how tsc (TypeScript compiler) should process and "compile" the source code. We need to use a command line tool for the sake of automatic checking, which we can include in the build on Azure Pipelines.

An alternative solution would be to simply use the options and filespec on the command line, and this is what I sometimes do for a quick check. But I think this is not what we want in the end for a couple of reasons.

I had some trouble with path wildcards on Windows, and it does not seem to work with globs at all. Please correct me if I am mistaken here. I found and tried a tsc-glob utility, does not seem to work at all with TypeScript 3 on Windows or Linux.

It is also desired to include all .js files by default, using tsconfig.json to specify what to exclude instead.

Thanks again to @rbiggs for helping me get started with this work.

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

@thorn0 and @brodybits, I never use a tsconfig file with my projects, and I use both live type linting and build type tests with tsc. You can take a look at a whole bunch of projects I have setup like this to see how I'm doing: https://github.com/composi/examples. Especially the projects in 2-complex projects/8-tour-of-heroes has quite complex custom types.

@thorn0
Copy link
Member

thorn0 commented Oct 25, 2019

@rbiggs jsconfig.json is really the same thing as tsconfig.json with two differences: 1) allowJs is implicitly set to true, 2) tsc and other tools don't find it automatically (only VS Code finds it, I guess). In your projects, you pass the compiler options as command line parameters, but that's what tsconfig.json exists for.

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

OK, my bad. Created a proper tsconfig.json for one of my projects and put all the options in there. Now all I needed for my NPM package script is : "test": "tsc". Nice :-)

@thorn0
Copy link
Member

thorn0 commented Oct 25, 2019

Yep! jsconfig seems to be just the VS Code team's tactical ruse aimed at the users who are afraid of the letters 'ts'. :)

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

No, jsconfig.json is specifically to let VSCode understand the structure of JavaScript only projects so it can provide better services like Intellisense, building, etc.

@thorn0
Copy link
Member

thorn0 commented Oct 25, 2019

But tsconfig does that too...

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

The jsconfig is for VSCode's JavaScript language service: https://code.visualstudio.com/docs/languages/jsconfig. If you're not doing any kind of type checking/linting all you need is a jsconfig file.

@Shinigami92
Copy link
Member

The jsconfig is for VSCode's JavaScript language service: https://code.visualstudio.com/docs/languages/jsconfig. If you're not doing any kind of type checking/linting all you need is a jsconfig file.

So we have the answer 🎉
We want type checking (linting could also be possible in the future)
Now lets use tsconfig

@thorn0
Copy link
Member

thorn0 commented Oct 25, 2019

That's what they write there. But in fact, this so called JavaScript language service is nothing else than the good old TS language service. As we know, it supports JS, so technically it is a JS language service too, no cheating! They just don't want to write that on that page to not scare anyone off. :)

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

True, but most jsconfig files only have two or three settings in them, compared to a typical tsconfig file. And try to explain to someone using Flow in VSCode that they need a tsconfig file. Not gonna fly. So, ta-da! jsconfig.

@thorn0
Copy link
Member

thorn0 commented Oct 25, 2019

Flow has its own incompatible syntax and thus a language service, but you got my point.

@thorn0 thorn0 merged commit 7eb4431 into prettier:master Oct 25, 2019
@rbiggs
Copy link

rbiggs commented Oct 25, 2019

I just noticed something. Removed the jsconfig from one of my projects. Seemed to work alright with just the tsconfig file. But then problems started when I was using imported JavaScript functions. VSCode had no idea how to import them with just the tsconfig file and only offered to ignore them. Adding the jsconfig file fixed this problem. So I'm guess that VSCode uses the jsconfig file to understand JavaScript module imports, something it does do with just the tsconfig file.

@thorn0
Copy link
Member

thorn0 commented Oct 25, 2019

Didn't you forget to copy some configuration from jsconfig to tsconfig?

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

Nope, they have the same file information. Just tsconfig has all the extra options for tsc. Otherwise the rest is the same. Go figure.

I used to be able to just type a function and hit enter to get an automatic import. Without jsconfig that's not working now.

Putting the jsconfig back in fixes the automatic import of JavaScript files.

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

Ah, just so you guys know, my tsconfig had "module": "commonjs". Removing that fixed the problem. Now with just the tsconfig everything works fine. It's always something that appears innocuous that deals the death blow.

@rbiggs
Copy link

rbiggs commented Oct 25, 2019

Actually, it was because my tsconfig file had "module": "commonjs". Removing that fixed the problem and auto imports are working again.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants