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

Is it possible to move an abstract base classes / base interface into a private module #2159

Closed
1 of 4 tasks
shivlaks opened this issue Oct 21, 2020 · 4 comments
Closed
1 of 4 tasks
Labels
guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged.

Comments

@shivlaks
Copy link
Contributor

shivlaks commented Oct 21, 2020

❓ Guidance

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)

General Information

  • JSII Version: 1.13.0
  • Platform:: OS X

The Question

It's common within a CDK construct library to have an abstract class that is not exported and only concrete implementations are exported.

When attempting to move the base class to it's own file in aws/aws-cdk#10925, JSII hits me with a warning
during build time that precludes the ability to move it to a folder such as .private

The desired outcome is a better organization of code where each concrete implementation has it's own file.
Similarly, if there is a props interface that the concrete implementations extend from, the same error message is presented.

The error message indicates that this may be by design. Is this a limitation that can be remediated or is it fundamentally required that classes be structured where the base class is in the same file as concrete implementations?

Error Message:

Unable to resolve type "@aws-cdk/aws-cognito.UserPoolIdentityProviderBase". It may be @iternal or not exported from the module's entry point (as configured in "package.json" as "main").

@shivlaks shivlaks added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Oct 21, 2020
@shivlaks shivlaks changed the title Is it possible to move an abstract base classes into a private module Is it possible to move an abstract base classes / base interface into a private module Oct 21, 2020
@RomainMuller
Copy link
Contributor

Basically, jsii is unable to determine whether you decided/meant not to export this type from your module, or whether you forgot to export it. This could result in weird API breakage if you later realize your error and fix that, so jsii prefers to complain rather than ignore.

What you are trying to doc should be achievable by annotating your "private" types as /** @internal */, which is the way jsii is notified that you intended for this declaration not to be visible outside of the module.

@RomainMuller
Copy link
Contributor

This is a duplicate of #1980 -- the main thing here is the error message is bad. This needs to be improved.

@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@shivlaks
Copy link
Contributor Author

@RomainMuller when I added @internal initially, the yarn package step still produced the base class in the generated code packages. I'll revisit it now, is there a better / more correct way to verify that the intended behaviour is still achieved?

mergify bot pushed a commit to aws/aws-cdk that referenced this issue Nov 6, 2020
…hat base class is not exported (#11056)

This reverts commit from #10925 which folded all implementations of `UserPoolIdentityProviderBase`
into a single file.

It was desirable to maintain our original code organization but we also did not want to export the base class
In light of guidance provided in aws/jsii#2159,  reverted back to original code organization and added
the `@internal` decorator on the base class in a private directory

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

2 participants