Fix types for gjs extensions with explicit imports #134
Conversation
src/parser/gjs-gts-parser.js
Outdated
| ); | ||
| return multiValueResolver(values, 'project'); | ||
| } else { | ||
| // For allowArbitraryExtensions - return first found value |
There was a problem hiding this comment.
why is this comment here? the code doesn't do anything with any specific option
There was a problem hiding this comment.
Looks like something that got left behind on accident.
src/parser/gjs-gts-parser.js
Outdated
| if (tsconfigPaths.length > 0) { | ||
| const allowJsValues = tsconfigPaths.map((cfg) => parseAllowJsFromTsconfig(cfg, rootDir)); | ||
| return resolveAllowJs(allowJsValues, 'project'); | ||
| if (multiValueResolver) { |
There was a problem hiding this comment.
this multi-value resolver feels a bit silly.
It's only used by getAllowJs, and no other code cares - so this makes the factoring of this function feel a bit wierd.
Like, the tasks of collecting tsconfigs feels different from getting the value from a collection of tsconfigs.
at the same time, I almost think having a whole control from for multiValueResolver is silly, and we should get rid of it, but instead have a different mechanism for warning when tsconfigs have conflicting values.
atm, this feels like its trying to be too generic, but is actually a very specific use case, so it's an unneeded abstraction (so far).
I would get rid of the multiValueResolver entirely.
And instead, have a hard-coded list of properties that we want to warn about conflicting values for.
Then, in the (currently) else branch below, do:
if (WARN_ON_CONFLICT.has(property)) {
printWarnIfConflict(tsconfigPaths, rootDir, property);
}
for (const tsconfigPath of tsconfigPaths) {
const result = parseCompilerOptionFromTsconfig(tsconfigPath, rootDir, property);
if (result !== undefined) return result;
}There was a problem hiding this comment.
Yeah, this was probably the AI over optimizing.
| const programAllowArbitraryExtensions = | ||
| result.services.program.getCompilerOptions?.()?.allowArbitraryExtensions; | ||
| if ( | ||
| !allowArbitraryExtensionsWasSet && |
There was a problem hiding this comment.
how does this situation happen?
There was a problem hiding this comment.
I'm not sure. I wish I knew! I've seen it actually happen with allowGjs.
| } | ||
| if (fileName.endsWith('.gts') || (allowGjs && fileName.endsWith('.gjs'))) { | ||
| // Only transform template files, not declaration files | ||
| if ( |
There was a problem hiding this comment.
does this need to account for .d.gjs.ts?
There was a problem hiding this comment.
No, but we should clean it up. Since a file can't both end with .gts and also end with .d.ts.
There was a problem hiding this comment.
is this a relevant test file?
There was a problem hiding this comment.
I suppose ts importing gjs is good, ya
| Allow importing files with arbitrary extensions like .d.gjs.ts | ||
| */ | ||
| "allowArbitraryExtensions": true, |
There was a problem hiding this comment.
should flat-ts actually be duplicated to a different folder (flat-ts-arbitrary-extensions) -- since we have control flows that run when this is false, I think we should retain that, and create a separate test project for this use case -- all the new files can be moved in to that new project
| @@ -1,2 +1,2 @@ | |||
| const Bar = () => <template></template>; | |||
| const Bar = () => {return <template></template>}; | |||
| expect(importDeclarations[1].specifiers[0].imported.name).toBe('ApiClient'); | ||
| expect(importDeclarations[1].specifiers[1].imported.name).toBe('DefaultClient'); | ||
|
|
||
| // Verify that our extension replacement transforms .gjs to .mjs for TypeScript processing |
There was a problem hiding this comment.
why combine these assertions with the import verification assertions in this test?
feels like testing replaceExtensions is sort of a different thing altogether than the importDeclarations testing above
| const { patchTs, replaceExtensions } = require('../src/parser/ts-patch'); | ||
| patchTs({ allowGjs: true }); | ||
|
|
||
| const transformedCode = replaceExtensions(codeWithComplexGjsImports); |
| (!fileName.endsWith('.d.ts') && fileName.endsWith('.ts')) || | ||
| fileName.endsWith('.gts') || | ||
| (allowGjs && fileName.endsWith('.gjs')) | ||
| (fileName.endsWith('.gts') && !fileName.endsWith('.d.ts')) || |
There was a problem hiding this comment.
We should clean this up too.
2 participants
We already fixed types for extension-less imports but this fixes explicit extensions.