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

References are lost due to incorrect merge during creation of schema list #31

Open
Chaitrasaurav opened this issue Aug 4, 2024 · 13 comments

Comments

@Chaitrasaurav
Copy link

Hello,

We encountered a problem while testing one of our components and traced it back to this file: GitHub link. The issue arises from the merging of lodash, which doesn't merge arrays as expected. You can find more details on the issue here.

Example interface:

{
    deals: {
        content: (rail | dealsSelection)[]
    };
    teaser: {
        content: (usp | teaserText)[]
    }
}

Below is the screenshot of schema created:
image

As you can see, both "deals" and "teaser" contain "content". When the schema is merged, it only retains the last "content", causing one of the references to be lost.

Our suggested fix, which worked for us while testing:
entryReferences = (0, lodash_1.mergeWith)(entryReferences, schema[this.types.references], (obj, src) => { if (lodash_1.isArray(obj)) { return Array.from(new Set(obj.concat(src))); } } );

Please let me know if this isn't clear. We can arrange a meeting to discuss the issue in more detail if needed. Thank you!

Regards,
Chaitra

@aman19K
Copy link
Contributor

aman19K commented Aug 5, 2024

Thanks @Chaitrasaurav , for raising this issue. Once the problem has been resolved, we'll notify you.

@idmedia-kaiser
Copy link

Any update on this? @aman19K

@cs-raj
Copy link
Contributor

cs-raj commented Oct 8, 2024

Hi @idmedia-kaiser we will be releasing this on by 14th October

@idmedia-kaiser
Copy link

idmedia-kaiser commented Oct 9, 2024

Hi @cs-raj @harshithad0703 as I mentioned in #35 only replacing merge with mergeWith does not fixed the issue as the mergeWith function then behaves exactly like merge if no customizer is given. Please check lodash docs. You need to add a customizer function that concats the arrays, and best case as Set so you don't have duplicates in it. That's why I have added in my local fix.
(obj, src) => { if (isArray(obj)) { return Array.from(new Set(obj.concat(src))); }}

@idmedia-kaiser
Copy link

idmedia-kaiser commented Oct 9, 2024

Example would be

const a = { buttons: ['deals_filter_tag'], content: ['rails', 'deals_selection']};
const b = { content: ['usp'] };
const res = _.mergeWith(a, b, (obj, src) => {  
if (_.isArray(obj)) {  
	return Array.from(new Set(obj.concat(src))) 
}}
); 
console.log(res);

Gives you

{
  buttons: ["deals_filter_tag"],
  content: ["rails", "deals_selection", "usp"]
}

Same without the customizer

const a = { buttons: ['deals_filter_tag'], content: ['rails', 'deals_selection']};
const b = { content: ['usp'] };
const res = _.mergeWith(a, b); 
console.log(res)

gives you

{
  buttons: ["deals_filter_tag"],
  content: ["usp", "deals_selection"]
}

@idmedia-kaiser
Copy link

idmedia-kaiser commented Oct 29, 2024

@cs-raj @aman19K Should this issue now be fixed , as I saw the PR was modified as requested and merged two weeks ago. ?
We are curious because this part of the code was also removed?
for (const path in schema[this.types.assets]) { paths.push(path) }
#4578946#diff-1ebd2ec0fa0f7b0aac61f8683a96f14f9ba2f36cc068918c16b6c3aaf8418e0aL1442

@cs-raj
Copy link
Contributor

cs-raj commented Oct 29, 2024

Yes the issue should be fix now

@cs-raj cs-raj closed this as completed Oct 29, 2024
@idmedia-kaiser
Copy link

@cs-raj Can you please comment on my question, why this code block for (const path in schema[this.types.assets]) { paths.push(path) } was removed?

@cs-raj
Copy link
Contributor

cs-raj commented Oct 29, 2024

Hi @idmedia-kaiser we are checking this internally for the time being please don't use the latest version

@cs-raj cs-raj reopened this Oct 29, 2024
@idmedia-kaiser
Copy link

Okay. @cs-raj maybe you can fix #42 in the same release :)

@idmedia-kaiser
Copy link

Hi @cs-raj @aman19K How long do we need to wait now for the fix of the fix? :)

@harshithad0703
Copy link
Contributor

Hi @idmedia-kaiser
We've resolved the original issue, but due to the pending fix for issue #42, we haven't released it yet. Rest assured, we're working on it and will try to release it as soon as possible.

Could you also check this review comment

@cs-raj
Copy link
Contributor

cs-raj commented Dec 18, 2024

Hi @idmedia-kaiser we will be releasing the 09-01-2025. We were not able to release it early due to freeze period.
cc: @aman19K @harshithad0703

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

No branches or pull requests

5 participants