-
Notifications
You must be signed in to change notification settings - Fork 58
feature: add support for single file type checking #942
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
base: main
Are you sure you want to change the base?
Changes from all commits
4961e94
138598b
feedfd2
b569534
d9724e7
b05380e
87def40
711e653
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,37 @@ | ||
| import { runTsc } from '@volar/typescript/lib/quickstart/runTsc.js'; | ||
| import { createEmberLanguagePlugin } from '../volar/ember-language-plugin.js'; | ||
| import { findConfig } from '../config/index.js'; | ||
| import { findConfig, createTempConfigForFiles, findTypeScript } from '../config/index.js'; | ||
|
|
||
| import { createRequire } from 'node:module'; | ||
| import { LanguagePlugin, URI } from '@volar/language-server'; | ||
| import { runTscWithArgs } from './utils.js'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
|
|
||
| export function run(): void { | ||
| let cwd = process.cwd(); | ||
| const cwd = process.cwd(); | ||
| const args = process.argv.slice(2); | ||
|
|
||
| // Use TypeScript's built-in command line parser | ||
| const ts = findTypeScript(cwd); | ||
|
|
||
| if (!ts) { | ||
| throw new Error('TypeScript not found. Glint requires TypeScript to be installed.'); | ||
| } | ||
|
|
||
| const parsedCommandLine = ts.parseCommandLine(args); | ||
|
|
||
| // Handle parsing errors | ||
| if (parsedCommandLine.errors.length > 0) { | ||
| parsedCommandLine.errors.forEach((error) => { | ||
| console.error(ts.flattenDiagnosticMessageText(error.messageText, '\n')); | ||
| }); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const files = parsedCommandLine.fileNames; | ||
| const compilerOptions = parsedCommandLine.options; | ||
| const hasSpecificFiles = files.length > 0; | ||
|
|
||
| const options = { | ||
| extraSupportedExtensions: ['.gjs', '.gts'], | ||
|
|
@@ -21,16 +46,48 @@ export function run(): void { | |
| // See discussion here: https://github.com/typed-ember/glint/issues/628 | ||
| }; | ||
|
|
||
| const main = (): void => | ||
| runTsc(require.resolve('typescript/lib/tsc'), options, (ts, options) => { | ||
| const glintConfig = findConfig(cwd); | ||
| const createLanguagePlugin = (): LanguagePlugin<URI>[] => { | ||
| const glintConfig = findConfig(cwd); | ||
| return glintConfig ? [createEmberLanguagePlugin(glintConfig)] : []; | ||
| }; | ||
|
|
||
| if (hasSpecificFiles) { | ||
| // For specific files, create temporary tsconfig that inherits from project config | ||
| const { tempConfigPath, cleanup } = createTempConfigForFiles(cwd, files); | ||
|
|
||
| try { | ||
| // Build TypeScript arguments for single file checking | ||
| const tscArgs = ['node', 'tsc', '--project', tempConfigPath]; | ||
|
|
||
| // Convert compiler options back to command line arguments | ||
| // Skip conflicting options that we control | ||
| const filteredOptions = { ...compilerOptions }; | ||
| delete filteredOptions.project; | ||
|
|
||
| // Add --noEmit as default only if user hasn't specified emit-related flags | ||
| const hasEmitFlag = Boolean( | ||
| compilerOptions.noEmit || | ||
|
Comment on lines
+68
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should support emitting at all in this mode. You'd get a partial project, which, if you're a library, would be bad for your consumers. Applications don't have any reason to emit at all, iirc
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to support emit, as long as the default is But whatever you say, I'm more or less fine with either way. |
||
| compilerOptions.declaration || | ||
| compilerOptions.emitDeclarationOnly || | ||
| compilerOptions['build'], | ||
| ); | ||
|
|
||
| if (glintConfig) { | ||
| const gtsLanguagePlugin = createEmberLanguagePlugin(glintConfig); | ||
| return [gtsLanguagePlugin]; | ||
| } else { | ||
| return []; | ||
| if (!hasEmitFlag) { | ||
| filteredOptions.noEmit = true; | ||
| } | ||
| }); | ||
| main(); | ||
|
|
||
| // Convert options back to command line format | ||
| const compilerArgs = Object.entries(filteredOptions) | ||
| .filter(([, value]) => value !== false && value !== undefined) | ||
| .flatMap(([key, value]) => (value === true ? [`--${key}`] : [`--${key}`, String(value)])); | ||
|
|
||
| tscArgs.push(...compilerArgs); | ||
|
|
||
| runTscWithArgs(require.resolve('typescript/lib/tsc'), tscArgs, options, createLanguagePlugin); | ||
| } finally { | ||
| cleanup(); | ||
| } | ||
| } else { | ||
| runTsc(require.resolve('typescript/lib/tsc'), options, createLanguagePlugin); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { runTsc } from '@volar/typescript/lib/quickstart/runTsc.js'; | ||
| import type { LanguagePlugin, URI } from '@volar/language-server'; | ||
|
|
||
| /** | ||
| * Helper function to run tsc with custom arguments while safely managing process.argv. | ||
| * This encapsulates the process.argv mutation to avoid polluting global state. | ||
| */ | ||
| export function runTscWithArgs( | ||
| tscPath: string, | ||
| args: string[], | ||
| options: any, | ||
| createLanguagePlugin: () => LanguagePlugin<URI>[], | ||
| ): void { | ||
| const originalArgv = process.argv; | ||
| try { | ||
| process.argv = args; | ||
| runTsc(tscPath, options, createLanguagePlugin); | ||
| } finally { | ||
| // Always restore original argv, even if runTsc throws | ||
| process.argv = originalArgv; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import * as fs from 'node:fs'; | ||
| import * as os from 'node:os'; | ||
| import * as path from 'node:path'; | ||
| import { describe, beforeEach, afterEach, test, expect } from 'vitest'; | ||
| import { createTempConfigForFiles } from '@glint/core/config/loader'; | ||
|
|
||
| describe('CLI: single file checking', () => { | ||
| const testDir = `${os.tmpdir()}/glint-cli-test-${process.pid}`; | ||
|
|
||
| beforeEach(() => { | ||
| fs.rmSync(testDir, { recursive: true, force: true }); | ||
| fs.mkdirSync(testDir, { recursive: true }); | ||
|
|
||
| // Create a minimal tsconfig.json | ||
| fs.writeFileSync( | ||
| path.join(testDir, 'tsconfig.json'), | ||
| JSON.stringify( | ||
| { | ||
| compilerOptions: { | ||
| target: 'ES2015', | ||
| module: 'commonjs', | ||
| strict: true, | ||
| }, | ||
| glint: { | ||
| environment: 'ember-loose', | ||
| }, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ); | ||
|
|
||
| // Create a test file | ||
| fs.writeFileSync( | ||
| path.join(testDir, 'test.gts'), | ||
| `import Component from '@glimmer/component'; | ||
|
|
||
| export default class Test extends Component { | ||
| <template>Hello World!</template> | ||
| }`, | ||
| ); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| fs.rmSync(testDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| test('creates temp config for single file', () => { | ||
| const { tempConfigPath, cleanup } = createTempConfigForFiles(testDir, ['test.gts']); | ||
|
|
||
| try { | ||
| // Check temp config exists | ||
| expect(fs.existsSync(tempConfigPath)).toBe(true); | ||
|
|
||
| // Check temp config content | ||
| const tempConfig = JSON.parse(fs.readFileSync(tempConfigPath, 'utf-8')); | ||
| expect(tempConfig.files).toEqual(['test.gts']); | ||
| expect(tempConfig.include).toBeUndefined(); | ||
| expect(tempConfig.exclude).toBeUndefined(); | ||
| expect(tempConfig.compilerOptions.target).toBe('ES2015'); | ||
| expect(tempConfig.glint.environment).toBe('ember-loose'); | ||
| } finally { | ||
| cleanup(); | ||
| } | ||
|
|
||
| // Check cleanup worked | ||
| expect(fs.existsSync(tempConfigPath)).toBe(false); | ||
| }); | ||
|
|
||
| test('creates temp config for multiple files', () => { | ||
| // Create another test file | ||
| fs.writeFileSync( | ||
| path.join(testDir, 'test2.gts'), | ||
| `import Component from '@glimmer/component'; | ||
|
|
||
| export default class Test2 extends Component { | ||
| <template>Hello Second!</template> | ||
| }`, | ||
| ); | ||
|
|
||
| const { tempConfigPath, cleanup } = createTempConfigForFiles(testDir, [ | ||
| 'test.gts', | ||
| 'test2.gts', | ||
| ]); | ||
|
|
||
| try { | ||
| const tempConfig = JSON.parse(fs.readFileSync(tempConfigPath, 'utf-8')); | ||
| expect(tempConfig.files).toEqual(['test.gts', 'test2.gts']); | ||
| } finally { | ||
| cleanup(); | ||
| } | ||
| }); | ||
|
|
||
| test('handles missing tsconfig', () => { | ||
| fs.unlinkSync(path.join(testDir, 'tsconfig.json')); | ||
|
|
||
| expect(() => { | ||
| createTempConfigForFiles(testDir, ['test.gts']); | ||
| }).toThrow('No tsconfig.json found'); | ||
| }); | ||
|
|
||
| test('cleanup is fired', () => { | ||
| const { tempConfigPath, cleanup } = createTempConfigForFiles(testDir, ['test.gts']); | ||
|
|
||
| // Cleanup once | ||
| cleanup(); | ||
| expect(fs.existsSync(tempConfigPath)).toBe(false); | ||
|
|
||
| // Cleanup again - should not throw | ||
| expect(() => cleanup()).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have numbers on the size of your project and how much of a difference this made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, > 5000 files.
Running the
glintcommand on all files can take up to 60 seconds (when used with lint-staged). Since we're experiencing issues with the Glint extension frequently crashing, broken code sometimes gets pushed upstream, and TypeScript errors are only caught later when the full command is executed in CI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint staged has a glint plugin? Do you have to use that?
Also, have you tried the v2 alphas and prererease in vscode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for specific plugins, you can use it like this.
I wouldn't say we have to use it that way, but it would be super convenient if, along with other linters, it could run glint (lint) on the changed files at commit time together with stylelint, for example, and the others...
I don't think so...
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, but typescript doesn't work this way, it has to resolve the types of everything imported.
But! I will concede if with this branch, the outputs are significantly different:
If you could get me these numbers, that'd be a huge help
You'll want to, as we've frozen glint v1 and are only working on glint v2 right now. It's already way faster.
(And this pr is targeting v2)
Caveat tho, we dropped support for hbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It correctly detects the errors I intentionally introduce, so everything seems to be working fine.
Also, the 19 seconds is because I ran Glint in just one project (we have a monorepo with multiple projects and packages), so it would take even longer otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty good results!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, a file with multiple imports will resolve and lint all those imported files as well. But even so, it’s still significantly faster than always running the Glint command across the entire codebase.