Skip to content
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

Reintroduce type splitting in NodeJS codegen. #11579

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RobbieMcKinstry
Copy link
Contributor

Description

This commit reintroduces the changes that were reverted in #11534. This time, we hide the feature behind language-specific config in the schema, and default it to off.

At this time, providers should not enable the feature until #11560 has been addressed.

Note to reviewers: Observe that this change does not edit the codegen tests. This demonstrates the these changes do not impact the generated code (because the feature introduced here is off by default).

Fixes #11557

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

This commit reintroduces the changes that were reverted
in #11534. This time,
we hide the feature behind language-specific config in the schema,
and default it to off.

At this time, providers should not enable the feature until
#11560 has been addressed.
@RobbieMcKinstry RobbieMcKinstry added kind/bug Some behavior is incorrect or out of spec language/javascript area/codegen SDK-gen, program-gen, convert impact/regression Something that used to work, but is now broken labels Dec 7, 2022
@RobbieMcKinstry
Copy link
Contributor Author

Marking as draft until CI completes. I expect it to be otherwise ready, but no sense reviewing it before then.

@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2022-12-07)

Bug Fixes

  • [sdkgen/nodejs] reintroduce-types-splitting
    #11579

@RobbieMcKinstry
Copy link
Contributor Author

We know why CI is failing, and I made a PR to fix it. In the meantime, we can go ahead with code review.

@t0yv0
Copy link
Member

t0yv0 commented Dec 12, 2022

At this time, providers should not enable the feature until #11560 has been addressed.

Wondering perhaps this feature does not belong on the main branch perhaps until that's addressed? That is don't merge this PR? Or is there a use case where a provider would want to enable it?

@RobbieMcKinstry
Copy link
Contributor Author

Wondering perhaps this feature does not belong on the main branch perhaps until that's addressed?

We can do that instead. My concern is that when we do eventually merge from the branch back into master, the PR will be massive. That doesn't inconvenience me (I can keep rebasing as needed) but it might be tough for reviewers.

@RobbieMcKinstry
Copy link
Contributor Author

Converting back into draft for now based on that comment.

@RobbieMcKinstry RobbieMcKinstry marked this pull request as draft December 13, 2022 13:29
@t0yv0
Copy link
Member

t0yv0 commented Dec 13, 2022

I've used a stacked PR technique in cases like this but it does still get unwieldy.. The advantage of keeping things out is that when you are debugging a prod issue you are not looking at code that's not in use. Really depends on how the team decides to handle these situations 🙏

@RobbieMcKinstry
Copy link
Contributor Author

Yeah, that makes a lot of sense. I'll go with a stacking PRs approach and give that a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen SDK-gen, program-gen, convert impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec language/javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[codegen/node] Add config to enable type splitting
3 participants