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

Decision: Support Complex Lists #958

Closed
DanielMSchmidt opened this issue Sep 8, 2021 · 5 comments
Closed

Decision: Support Complex Lists #958

DanielMSchmidt opened this issue Sep 8, 2021 · 5 comments
Labels
enhancement New feature or request feature/tokens

Comments

@DanielMSchmidt
Copy link
Contributor

DanielMSchmidt commented Sep 8, 2021

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

My thinking is that as we want to stick with native types we need to get rid of ComplexComputedList and rather have sth to the effect of ComplexComputedListItems (just changing the structure to return an array of such a class instance instead of the class to stay in the native language array realm.

Alternative A

We can make all lists arrays of string that we cast to the right interface. To use elements the user would need to user Terraform functions, so the types are only for educational purposes.

Alternative B

To make this work we would need to check when resolving an array if its single item is extending from ComplexComputedListItem and resolve this accordingly. If a single item is detected we should throw an error similar to the one introduced in #903 since we can only allow list access through functions as of right now.

Alternative C

Alternatively we could make a bit more guessing work for a great UX. We could always create a list of n items (where n is a huge number) and have each item carry an index, so that just taking the first / nth item works. We could even make getting the last one work by some special case handling. We could detect if all items are in a list and resolve to a *.

If we go with that approach we could also try to guess in a similar way when folks natively iterate over lists, we could collect "markers" for each list if all / a few (e.g. first or last or first two, whatever we deem appropriate). We could detect if folks use a weird amount (e.g. every second, which will not work since we don't know the length of the list) and throw an error to get them to use terraform functions instead. This could be implemented via annotations on Construct nodes and a visitor that pre-processes the Graph so that we can get rid of the nodes created by looping over a native array and replace it with a representation of the sub-elements with a for_each / count property.

Alternative D

@ansgarm proposed we could use a builder-like pattern to give users a strongly-typed functional interface for expressing their looping / accessing logic. While this would not allow us to interact with the lists natively and we would loose all autocompletion we can express a lot more fine-grained logic by composing these functions together. An example might look like this:

forEach(myVpc.subnet_ids, [createResource(this, "name", (id ) => (...)])
forEach(myDataSource.elements, [conditional(element => element.isActive, element => createResource(...), null)])
Solution Proper Interface in all languages Can be passed as an argument Has Type representing content Can be natively iterated over Single element can be accessed natively Is safe from unintended outcomes Can express complex runtime conditionals
Current Solution 🚫 🚫 🚫 🚫 🚫
A) Casted String-Array 🚫 🚫 🚫 🚫
B) Single Item Array 🚫 🚫 🚫
C) N-Item Array 🚫 (reverse-engineering the users code can result in wrong guesses) 🚫
D) Builder-Pattern access 🚫 🚫

References

@DanielMSchmidt DanielMSchmidt added enhancement New feature or request feature/tokens labels Sep 8, 2021
@jsteinich
Copy link
Collaborator

Perhaps not an N element array, but the general idea of approach C is my dream for what this could be. It's an ambitious undertaking and could certainly get things wrong, so I think it's best to start with another approach.

The builder approach has some nice properties, but I think may break down when it comes to actual implementation. A lack of generics means code generation is needed to provide useful type information, but certain operations would involve multiple types which very quickly becomes a mess.

The casting approach is asking for trouble since it may work fine for typescript when simply passing references, but breaks at the jsii boundary when types don't match up.

That leaves approach B. I'm not a huge fan of the error, but I think it does make sense.

If accessing a particular element is a very common option, then we could go with the option described in #25 instead.

@DanielMSchmidt DanielMSchmidt changed the title Support Complex Lists Decision: Support Complex Lists Sep 10, 2021
@DanielMSchmidt
Copy link
Contributor Author

We decided to go with approach C (maybe using B as a stepping stone).
Created #993 and #994 to track the work

@skorfmann
Copy link
Contributor

We decided to go with approach C (maybe using B as a stepping stone).

Is there more detailed reasoning available somewhere? /cc @DanielMSchmidt

@DanielMSchmidt
Copy link
Contributor Author

We discussed that C feels the most like using a native language and that we could limit the edge cases folks run into and if they still do they can use an escape hatch as a workaround

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2022

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request feature/tokens
Projects
None yet
Development

No branches or pull requests

3 participants