Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add migrations for new block editor #7910
Add migrations for new block editor #7910
Changes from all commits
f78e4fc
d6c95bb
432ce80
ff8ef00
e7be4bd
133d307
e8a31ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just do
Database.Fetch<ContentTypeDto>().ToDictionary(x => x.Alias, x => x.NodeId)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had written an app a couple years back that did a lot of list to dictionary conversions, and it was performing horribly. After doing using VS's performance analyzer, we found that the dictionary resize operations are extremely expensive, especially as the list grows to 1,000+ elements. If you size the dictionary appropriately at the start and then just insert into it, it was lightning fast. Ever since then, I've always tried to make it a habit to size the dictionary appropriately and then insert into it. Since the ToDictionary extension is on the IEnumerable interface, it can't do that, and so just iterates and adds to the dictionary. If they created a ToDictionary that was an extension on the ICollection interface, that would be ideal, but without that I just create my own, properly sized dictionary and add to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough! Might be worth making an ext method for this then based on ICollection, not required for this PR but in the future. And good to know! Wonder if there are places in our codebase where we are creating large dictionaries with ToDictionary based on collections, might have a look today and see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to create a PR for changing some of our internal usages of ToDictionary and discovered we should be using ToLookup in various places too instead of GroupBy/ToDictionary which is much slower. I've left this open for the community to review but please have a look i fyou have time #7969
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any chance this can break people's sites? Like if potentially they were using a doc type as both an element type and a doc type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is, and I thought about that further when I was writing the migrations that I did for #7939 where I decided that if there were any element types already (i.e. this wasn't a v7 to v8 migration), then I wouldn't mess with retyping any document types. I'll port that change back to here, to minimize the effect on anyone upgrading from a previous v8 version. We are dealing just with those upgrading Stacked Content, so likely they are still coming straight from v7 anyway, but it would be a good quick check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool so since this is already merged i supposed we will just wait until you have a little bit of time to push a new PR, just let us know :) I'll add some notes to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a follow-up to this, I ended up pulling the ConvertToElement migration in PR #8336 since the NestedContent portion is PR'd in #7957 and without the StackedContent to BlockEditorList migration, there is nothing else to convert.