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

fix: [Angular] fix logic used for parsing objects provided as input to useObjectWrapper #1490

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lukjab71
Copy link

@lukjab71 lukjab71 commented Jul 5, 2024

Description

This is propsed fix to issue: 1410

  • What changes you made:
    The logic responsible for parsing an input to the method was incorrect. It used ',' comma as object separator. This gave weird results for any other constructs that would use comma, like arrays. Besides, this logic should probably be used only when input actually has spread operator in it.

Regardless from object detection logic, I'm not convinced if logic responsible for concatenation ${spreadOutObjects.join(', ')}, ${otherObjs.join(', ')} was correct. spreadOutObjects collects objects that use spread operator. otherObjs collect the rest of the objects from input. When we concatenate them using the initial logic, I believe we are completely mixing the order of objects, as there is no guarantee that spreadOutObjects in the original input, where at the begining. What if they were spread out somewhere else, in some nested object, of some complicated input? I believe this would either completely malform the object or in best case change the object structure.

Now, about the propsed solution. As I understand, the whole purpose of this method is to get rid of ... spread operator from syntax, as angular won't accept it.

I tried to approach this problem in recursive way(to fix initial way of detecting objects - instead of using comma as object separator, try to figure start bracket and end bracket), but it is tricky, as regex are not good with recursive problems and even when I extracted recursion to separate method, it still was tricky for some complicated examples. And then I switched the approach - in the end, why do we need to detect separate objects? Our goal is to get rid of ... from the string. So we can simply get rid of the ... and then deal with potential consequences:

  1. someField: ...someObject -> remove ... and we are done
  2. someField: {...someObject} -> remove ... and we are left with {object} result
  3. someField: {...{nestedDeeper: true }} -> remove ... and we are left with {{ something: true }} result

I believe those cases will cover every possible variations on different levels of nesting. So after removing ... we just have to cleanup what's left.

I prepared the PR using remarks from CONTRIBUTING.md file, however, I noticed that even without my changes there are tests failing in the core package(?).

@lukjab71 lukjab71 requested a review from samijaber as a code owner July 5, 2024 06:42
Copy link

changeset-bot bot commented Jul 5, 2024

🦋 Changeset detected

Latest commit: 9ad3dab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@builder.io/mitosis Minor
@builder.io/mitosis-cli Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant