Allow diagnostic and completions in js #337
Conversation
|
Ah it seems that the tests are failing on some javascript specific testings. I'll need to check those out and iron out some of those still. |
|
There might be some cases we want to review here: One test includes a case "ignores js file", which checks that the JS file doesn't get any diagnostics info etc. However with this completion + diagnostic update we are getting diagnostic data for JS files and at least IMO that is a plus side. So... Should we just remove this test? ( |
|
The other failing test seems to be on " compiles jsx file to js" and the reason it fails is because it also gets diagnostics instead of just the compiler output. Should we just update the expected cases with the corresponding diagnostics info? |
| @@ -10,6 +10,8 @@ import {PackageJson} from './util.js'; | |||
| import {makeLspDiagnostic} from './diagnostic.js'; | |||
| import {WorkerContext} from './worker-context.js'; | |||
|
|
|||
| const PROCESSED_FILE_ENDINGS = ['.ts', '.jsx', '.tsx']; | |||
There was a problem hiding this comment.
| const PROCESSED_FILE_ENDINGS = ['.ts', '.jsx', '.tsx']; | |
| const COMPILED_FILE_ENDINGS = ['.ts', '.jsx', '.tsx']; |
(Just a nit, since .js is "processed", it's just not compiled)
| @@ -20,22 +22,32 @@ export async function* processTypeScriptFiles( | |||
| let packageJson: PackageJson | undefined; | |||
| const compilerInputs = []; | |||
| for await (const result of results) { | |||
| if (result.kind !== 'file') continue; | |||
There was a problem hiding this comment.
| if (result.kind !== 'file') continue; | |
| if (result.kind !== 'file') { | |
| continue; | |
| } |
Style nit
|
Hi @Matsuuu, is this something that you will be able to work on in the near future? Otherwise I can help continue the work. |
Ah thanks for the nudge @rwalle ! I had sorta forgotten about this. I'll try and set some time aside in the coming week to look into finalizing this. It's mostly just rewriting a couple of tests and testing it out |
| @@ -632,7 +632,7 @@ export class PlaygroundCodeEditor extends LitElement { | |||
| private _currentFiletypeSupportsCompletion() { | |||
| // Currently we are only supporting code completion for TS. Change | |||
| // this in a case that we start to support it for other languages too. | |||
| return this.type === 'ts'; | |||
| return this.type === 'ts' || this.type === 'js'; | |||
There was a problem hiding this comment.
Could we also support completions for tsx and jsx?
There was a problem hiding this comment.
Yeah I think we could see if it works out of the box with those too
|
I think I should grab myself from the bootstraps and get this thing actually finished finally. I remember having some hiccups with the env at the start of the year but I'll try and find some time to finalize this in the coming weeks. I'm still awaiting a little bit of feedback on some of my previous comments. Namely: There might be some cases we want to review here: One test includes a case "ignores js file", which checks that the JS file doesn't get any diagnostics info etc. However with this completion + diagnostic update we are getting diagnostic data for JS files and at least IMO that is a plus side. So... Should we just remove this test? (typescript-worker_test.ts:39) The other failing test seems to be on " compiles jsx file to js" and the reason it fails is because it also gets diagnostics instead of just the compiler output. Should we just update the expected cases with the corresponding diagnostics info? So basically both of these cases are just for the past where we didn't expect to have JS support, and also for cases where the diagnostics were lacking. Updating the test cases with the updated info would be beneficial IMO and removing some of the tests that want us to ignore JS would also but beneficial as that's not the case anymore. After I've gotten some closure on these, I'll try and find a time to finalize the PR and test it out so that we can ship JS (and maybe JS/TS(X) ) completions with the playground! |
|
Thanks so much @Matsuuu !
'index.d.ts': {
content:
'declare export = {createElement(tag: unknown, props: unknown, children: unknown) => unknown;}',
},The language service compiler option would need to include |
|
Yeah agree. I'll try and take a look at this in the coming weeks. Shouldn't be toomuch of a hassle |
This PR builds on top of #336 since the optimization was kind of needed for some of the code changes here too.
If this gets merged, we don't need to merge #336 since this was branched from it and the commit is here too.
The PR is quite simple in general: We just add JS as the allowed filetype in a couple of places and inform TS that javascript files should also be handled.
Then instead of just yielding the JS files in
processTypeScriptFileswe also add it to the compilerinputs. It will make it so that they are handled in the completions and diagnostics parts of our code, but the won't try to compile them since they're already javascript.