-
Notifications
You must be signed in to change notification settings - Fork 243
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
fix(go): unused imports emitted for type unions #3664
Changes from all commits
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 |
---|---|---|
|
@@ -137,9 +137,13 @@ export class GoTypeRef { | |
break; | ||
|
||
case 'union': | ||
for (const t of this.typeMap.value) { | ||
ret.push(...t.dependencies); | ||
} | ||
// Unions ultimately result in `interface{}` being rendered, so no import is needed. We | ||
// hence ignore them entirely here for now. In the future, we may want to inject specific | ||
// runtime type checks around use of unions, which may result in imports being useful. | ||
|
||
// for (const t of this.typeMap.value) { | ||
// ret.push(...t.dependencies); | ||
// } | ||
Comment on lines
+144
to
+146
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. This was left here intentionally as it will likely need to be re-introduced when @MrArnoldPalmer adds runtime type checks for type unions (#3641). |
||
break; | ||
|
||
case 'void': | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
{ | ||
"author": { | ||
"email": "aws-jsii@amazon.com", | ||
"name": "Amazon Web Services, Inc.", | ||
"organization": true, | ||
"roles": [ | ||
"owner" | ||
] | ||
}, | ||
"description": "A dummy test package", | ||
"fingerprint": "PHONY", | ||
"homepage": "https://aws.github.io/jsii", | ||
"jsiiVersion": "0.0.0-dev", | ||
"license": "Apache-2.0", | ||
"name": "base", | ||
"repository": { | ||
"directory": "packages/jsii-pacmak/test/targets/fixtures", | ||
"type": "git", | ||
"url": "https://github.com/aws/jsii" | ||
}, | ||
"schema": "jsii/0.10.0", | ||
"targets": { | ||
"go": { | ||
"moduleName": "github.com/aws/jsii/packages/jsii-pacmak/test/targets/fixtures" | ||
} | ||
}, | ||
"types": { | ||
"BaseA": { | ||
"assembly": "base", | ||
"fqn": "base.BaseA", | ||
"kind": "interface", | ||
"name": "BaseA" | ||
}, | ||
"BaseB": { | ||
"assembly": "base", | ||
"fqn": "base.BaseB", | ||
"kind": "interface", | ||
"name": "BaseB" | ||
} | ||
}, | ||
"version": "1.2.3" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
{ | ||
"author": { | ||
"email": "aws-jsii@amazon.com", | ||
"name": "Amazon Web Services, Inc.", | ||
"organization": true, | ||
"roles": [ | ||
"owner" | ||
] | ||
}, | ||
"dependencies": { | ||
"base": "1.2.3" | ||
}, | ||
"description": "A dummy test package", | ||
"fingerprint": "PHONY", | ||
"homepage": "https://aws.github.io/jsii", | ||
"jsiiVersion": "0.0.0-dev", | ||
"license": "Apache-2.0", | ||
"name": "dependent", | ||
"repository": { | ||
"directory": "packages/jsii-pacmak/test/targets/fixtures", | ||
"type": "git", | ||
"url": "https://github.com/aws/jsii" | ||
}, | ||
"schema": "jsii/0.10.0", | ||
"targets": { | ||
"go": { | ||
"moduleName": "github.com/aws/jsii/packages/jsii-pacmak/test/targets/fixtures" | ||
} | ||
}, | ||
"types": { | ||
"Dependent": { | ||
"assembly": "dependent", | ||
"fqn": "dependent.Dependent", | ||
"kind": "class", | ||
"name": "dependent", | ||
"methods": [ | ||
{ | ||
"name": "getBaseUnion", | ||
"returns": { | ||
"type": { | ||
"union": { | ||
"types": [ | ||
{ | ||
"fqn": "base.BaseA" | ||
}, | ||
{ | ||
"fqn": "base.BaseB" | ||
} | ||
] | ||
} | ||
} | ||
} | ||
} | ||
] | ||
} | ||
}, | ||
"version": "4.5.6" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import { promises as fs } from 'fs'; | ||
import { TypeSystem } from 'jsii-reflect'; | ||
import { Rosetta } from 'jsii-rosetta'; | ||
import { tmpdir } from 'os'; | ||
import { join } from 'path'; | ||
|
||
import { Golang } from '../../lib/targets/go'; | ||
|
||
test('does not generate imports for unused types', async () => { | ||
const outDir = await fs.mkdtemp(join(tmpdir(), 'pacmak-golang-')); | ||
try { | ||
const tarball = join(outDir, 'mock-tarball.tgz'); | ||
await fs.writeFile(tarball, 'Mock Tarball'); | ||
|
||
const typeSystem = new TypeSystem(); | ||
await typeSystem.load(require.resolve('./fixtures/base.jsii.json')); | ||
const assembly = await typeSystem.load( | ||
require.resolve('./fixtures/dependent.jsii.json'), | ||
); | ||
|
||
const rosetta = new Rosetta(); | ||
const subject = new Golang({ | ||
arguments: {}, | ||
assembly, | ||
packageDir: '', | ||
rosetta, | ||
targetName: 'golang', | ||
}); | ||
|
||
await subject.generateCode(outDir, tarball); | ||
|
||
await expect( | ||
fs.readFile(join(outDir, assembly.name, `${assembly.name}.go`), 'utf-8'), | ||
).resolves.not.toContain( | ||
'github.com/aws/jsii/packages/jsii-pacmak/test/targets/fixtures/base', | ||
); | ||
Comment on lines
+34
to
+36
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. Contemplated putting a |
||
} finally { | ||
await fs.rm(outDir, { recursive: true }); | ||
} | ||
}); |
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.
Note - this makes
jest --watch
a lot more pleasant to use, as it'd only re-run tests oncestsc --watch
updates the.js
files instead of possibly re-running when the.ts
file changed (which yields unexpected outcome).I'm contemplating moving this up to the baseline configuration but need to evaluate impact first.