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
Reduce types output #10831
Reduce types output #10831
Conversation
Changelog[uncommitted] (2022-11-11)Features
|
pkg/codegen/nodejs/gen.go
Outdated
// At this level, we export any nested definitions from | ||
// the next level. | ||
fmt.Fprintf(file.writer(), "import * as childname(%s) from \"childname(%s)\";\n", child.name, child.name) | ||
fmt.Fprintf(file.writer(), "export type child(%s);", child.name) |
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.
These are the re-exports? Interesting.
I'm a bit confused how the childname and child() render.
Including the results of applying this codegen change to examples (PULUMI_ACCEPT=1 go test -test.v TestGeneratePackage) in the PR can help illusrate.
Also I would be vary of export type
. Syntactically consider re-exports https://www.typescriptlang.org/docs/handbook/modules.html#re-exports
Semantically consider the multiple levels here. export type
will only export Type-level bindings. However I believe we might have value-level bindings in there as well? For example a class declaration has Type-level and Value-level bindings. There are also Namespace-level bindings. It's confusing.
I've just browsed through aws and tls types/{input,output}.ts. It would seem that most of the exports are exporting interfaces so export type
is fine. There's a few places that do export namespace
.
The second image shows how we're doing it now, and the first image shows how this PR reorganizes the files to achieve our goal.
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.
Sorry, I tried to indicate previously (but it's been awhile and I should have written it down in line!) that this is the major WIP part of the PR. It's just absolutely not the right path. childname
is a placeholder I added to help me trace the code.
I've just browsed through aws and tls types/{input,output}.ts. It would seem that most of the exports are exporting interfaces so export type is fine. There's a few places that do export namespace.
I think each export namespace
translates 1:1 with a new file created as part of this PR. When we create a new namespace
, we're really trying to create a new layer of nesting. See the images attached below for how this translates.
Forgive me if I'm not explaining this well, I can my articulation is suffering from a lack of clarity.
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.
Trying to take your suggestion to use the export * as x from "x"
syntax (from your linked resource) because you're right about the value-bindings. Right now it's looking like value bindings are still missing from those exports, which is strange, since they shouldn't be.
Looking promising! |
01f281a
to
4119c0e
Compare
Update: I'm successfully building AWS-Native with these changes. |
4119c0e
to
5d40c31
Compare
@RobbieMcKinstry did you get a chance to look at CI failures? For example this one: TestGeneratePackageTwo/plain-object-defaults/nodejs/compile Seems to be something off with finding the |
@@ -0,0 +1,7 @@ | |||
// *** WARNING: this file was generated by test. *** | |||
// *** Do not edit by hand unless you're certain you know what you are doing! *** |
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.
Special case for empty file to not emit empty modules? WDYT?
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 might be easier in a follow-on PR. Right now there are a lot of edge cases I'm coming up against regarding this same pattern. Before we merge, I can file an issue to handle this as a special case.
Suppose we implement a special case to omit an empty file. If either input or output module is empty, then the index file (which is generated separately) has to know that too, so that it doesn't generate an import for a file that isn't there.
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.
Actually, I suppose I can't open the issue until after this is merged, otherwise I'd be opening an issue about a problem that doesn't exist yet lol
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.
Yeah definitely not a blocker, just a nice to have.
@@ -5,33 +5,4 @@ import * as pulumi from "@pulumi/pulumi"; | |||
import * as inputs from "../types/input"; | |||
import * as outputs from "../types/output"; | |||
|
|||
export namespace documentdb { |
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.
Lovely. So the export *
replacement does the same thing as before? This looks right.
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.
Yeah, export * as ns from "file";
creates a namespace and re-exports everything exported by file
. I read about it in the v3.8 release notes:
export * as utilities from "./utilities";
is a syntactic sugar for:
import * as utilities from "./utilities";
export { utilities };
/** | ||
* typArgsProvideDefaults sets the appropriate defaults for TypArgs | ||
*/ | ||
export function typArgsProvideDefaults(val: TypArgs): TypArgs { |
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.
This is a concrete example of a non-type export in these inputs files. If we're still in doubt if re-export works as intended, we could check calling this engaging the re-export.
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.
Ah, the test plain-object-defaults
does exactly this. I had a name error on this very type until very recently.
I also removed the export type
syntax in favor of export * as ns
so I think its a nonissue.
I spent 3.5 hours chasing down a bug around inconsistent module import paths. I fixed it! I've got four failing tests left to get to:
|
pkg/codegen/testing/test/testdata/plain-object-defaults/nodejs/types/mod2/index.ts
Outdated
Show resolved
Hide resolved
310dfef
to
d737ae5
Compare
All codegen tests pass. Downstream codegen tests fail. I can't repro locally. However, I think the problem was a conflict in namespace declarations. I was exporting child modules from both input.ts and output.ts so there was a name conflict. Let's see if CI agrees with me. |
e1ea0ac
to
18942a5
Compare
🚀 Yay! I got tests to pass! ✨ Wow, this was incredibly difficult. Definitely the hardest PR I've worked on at Pulumi. |
A substantial piece of work, nice one @RobbieMcKinstry! I really appreciate that you kept commits to the point, and rebased to keep the history clean (there's a few WIP and reruns that you could |
Co-authored-by: Kyle Pitzen <kyle.pitzen@gmail.com>
72ba599
to
27da3fc
Compare
27da3fc
to
a07a250
Compare
bors r+ |
10831: Reduce types output r=RobbieMcKinstry a=RobbieMcKinstry <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description This is a draft PR for #8613. <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #8613 ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Robbie McKinstry <robbie@pulumi.com> Co-authored-by: Robbie McKinstry <thesnowmancometh@gmail.com>
Build failed: |
bors retry |
10831: Reduce types output r=RobbieMcKinstry a=RobbieMcKinstry <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description This is a draft PR for #8613. <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #8613 ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Robbie McKinstry <robbie@pulumi.com> Co-authored-by: Robbie McKinstry <thesnowmancometh@gmail.com>
Build failed: |
bors retry |
Build succeeded: |
Description
This is a draft PR for #8613.
Fixes #8613
Checklist
make changelog
and committed thechangelog/pending/<file>
documenting my change