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

Reject Circular References between Namespaces #270

Closed
dstufft opened this issue Oct 22, 2018 · 1 comment
Closed

Reject Circular References between Namespaces #270

dstufft opened this issue Oct 22, 2018 · 1 comment
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2

Comments

@dstufft
Copy link
Contributor

dstufft commented Oct 22, 2018

It appears (correct me if I'm wrong), that because namespaces in TypeScript are objects not actual modules, that something like:

export class Foo {
      public thing: bar.Baz;
}

export namespace bar {
    export class Baz {
        public other: Foo
    }
}

is possible to do?

Currently the Python code is structuring namespaces as individual modules because it does not have a "namespace" concept that is distinct from it's module system. This combined with code like the above, means that the Python code cannot generate workable code, because the top level module (say widget) would import from the submodule (widget.bar), and the sub module would then import from the top level module (widget).

At a minimum the JSII compiler should probably try to detect cycles like these, and reject them.

It might be worthwhile to consider being even stricter. It's generally, in my opinion, good form to have modules/namespaces like this not depend on something "higher" up in the tree (while depending on stuff "lower" in the tree is fine). That would generally mean (in my example above) that the module root could use anything from the bar namespace, but the bar namespace could not use anything from the module root.

This would likely entail needing to add some construct in the JSII to signal the need to re-export a type from one namespace to another namespace, because it appears the primary place we're violating this hypothetical rule, is when we want to share an attribute type between two different modules. It might be saner to implement that as a type defined in the "inner" namespace, and then explicitly re-exported from that namespace into the "outer" namespace.

eladb pushed a commit to aws/aws-cdk that referenced this issue Dec 13, 2018
Rename generated CloudFormation resource constructs from `cloudformation.XxxResource` to `CfnXxx`. This fixes #878 and eliminates the use of namespaces in the CDK.

This is done in a backwards compatible way, which means that we still generate the old resources under the `cloudformation` namespace so we can remove them in a subsequent release. Those resources also include a deprecation warning which is emitted upon `cdk synth`.

Documentation updated to reflect changes.

Related: aws/jsii#283 and aws/jsii#270
@fulghum fulghum added the p0 label Mar 12, 2019
@eladb eladb removed their assignment Jul 31, 2019
@eladb eladb removed the p0 label Jul 31, 2019
@SomayaB SomayaB added the feature-request A feature should be added or improved. label Nov 18, 2019
@SomayaB SomayaB added needs-triage This issue or PR still needs to be triaged. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 7, 2020
@RomainMuller RomainMuller added the effort/large Large work item – several weeks of effort label Jan 24, 2020
@RomainMuller RomainMuller removed their assignment Jun 24, 2021
@github-actions
Copy link
Contributor

This issue has not received any attention in 2 years. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

5 participants