-
Notifications
You must be signed in to change notification settings - Fork 456
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
feat(provider-generator): namespace every resource / data source / provider #2087
Conversation
This is a tough one. I certainly understand the motivation for doing this; however, the usage is definitely less clean. |
I measured through a subset of the examples to give us an idea of the improvements:
|
Yeah I agree, it's a trade-off. We will need to explore which APIs get worse / better and how much it impacts the compilation speed. In an ideal world I'd like to have the current API surface (and more) and fast synth / get times at the same time, but this seems to be impossible. This is one way we are looking into the issue, we'll also improve JSII directly, but that will most likely only affect the get times, not the synth times. One positive impact of this was autocompletion for python was working for the first time for me 🎉 I think the sub-modules make it easier for the language server to follow |
7be8214
to
2d0321e
Compare
Were the synth benchmarks for locally generated providers? Would be interesting to compare with the published bindings. While JSII has some fundamental slowness to it, we've seen some glaring performance issues so I'm optimistic that there is room for significant improvement there. Since this seems to be primarily an optimization, starting with profiling data to determine what aspects are slow should give more insight into the problem and better inform the choice. |
Locally, I haven't tested pre-built yet, but I think we need to. in the old bindings there was already a stark contrast between pre-built and local providers.
Yeah, we are also looking into the JSII performance, mostly for the
Do you mean profiling the synth operation? |
Yep. Should also profile the get operation, but that didn't feel like the focus of this PR. |
2d0321e
to
1e706d3
Compare
5f2f4b2
to
4e35b9d
Compare
Co-authored-by: Ansgar Mertens <ansgar@hashicorp.com>
…source as their namespace 'function' is not valid in TypeScript re-exports e.g. happens in the Scaleway Terraform Provider
7d10289
to
550d0cc
Compare
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This allows reducing the compile time across all languages since users only need to import
the used subset in the provider bindings instead of the entire one. Namespaces force JSII
to split the generated code up into multiple files that can be individually parsed and used.
I also put the provider in a namespaced and renamed the file from e.g.
aws-provider.ts
toprovider.ts
so that import paths are a bit nicer and namespaces areaws.provider.AwsProvider
vsaws.awsProvider.AwsProvider
.BREAKING CHANGE: This changes the generated API surface a lot, if you use an already namespaced provider (read: AWS) you only need to import the provider declaration differently. For any other provider the imports are changing so that users would need to change the imports for their resources / data sources to use namespaces or barrell imports.
TODOs
Document breaking change and migration guide(docs follow-up issue)Update all the docs(follow-up issue above)Documentation: add "Use specific / namespaced imports" in best practice section(follow-up issue above)Add doc string for each of the namespaces with a link to the TF docs(follow-up issue)