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

test(lib): ensure list mapper does not fail when passed IResolvables #1791

Merged
merged 4 commits into from
May 13, 2022

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented May 12, 2022

Closes #1788
Closes #910
Closes #835

Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@jsteinich
Copy link
Collaborator

I believe #835 has been fixed, but I'm not yet convinced that #910 still doesn't cause issues.

I haven't quite thought of a case where this would actually happen, but if deepMerge ends up mixing tokens and non-tokens within the same property the results could be wrong.
Another spot worth testing is Lazy. We don't really use it, but it does exist.

@DanielMSchmidt
Copy link
Contributor Author

if deepMerge ends up mixing tokens and non-tokens within the same property the results could be wrong.

on the same property as in different types of tokens in a list or as function invocations?

@jsteinich
Copy link
Collaborator

if deepMerge ends up mixing tokens and non-tokens within the same property the results could be wrong.

on the same property as in different types of tokens in a list or as function invocations?

A couple possible cases:

  • Trying to merge a token and non-token. One direct just overwrites, the other might append to the token object.
  • Adding token and non-token to list. Resolution will fail later.

@ansgarm there was a slack thread recently about JSON.stringify; would that line up with this.

@ansgarm
Copy link
Member

ansgarm commented May 12, 2022

Yeah, I just filed an issue for the JSON.stringify problem: #1796
There was also this one, but that is a separate issue: #1795

@DanielMSchmidt DanielMSchmidt merged commit 6987fe6 into main May 13, 2022
@DanielMSchmidt DanielMSchmidt deleted the reproduce-token-mapping-issue branch May 13, 2022 15:17
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Research] Token Resolution Issue bug(lib): Fix Token Resolve Property Mapper should Respect Tokens
3 participants