-
Notifications
You must be signed in to change notification settings - Fork 82
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
Update blockprotocol + mock-block-dock. Remove destinationEntityVersionId #553
Conversation
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.
Thanks Ciaran, I think one minor addition to the migration scripts should be made before merging for consistency, but apart from that these changes look good to me 👍
...ages/hash/datastore/postgres/migration/1652365970279_remove-destination-entity-version-id.ts
Outdated
Show resolved
Hide resolved
...ages/hash/datastore/postgres/migration/1652365970279_remove-destination-entity-version-id.ts
Outdated
Show resolved
Hide resolved
); | ||
const fieldsWithId = (multiFilter.filters ?? []).map((filter) => ({ | ||
...filter, | ||
id: uuid(), |
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 curious, why do these filters each have ids in the table block? Just for the purpose of using them in the react key?
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'm not sure - @teenoh ?
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.
we need the ids to locate a particular filter from the list of filters and update it's value when there's a change @benwerner01
…e-destination-entity-version-id.ts Co-authored-by: Ben Werner <42802102+benwerner01@users.noreply.github.com>
…e-destination-entity-version-id.ts Co-authored-by: Ben Werner <42802102+benwerner01@users.noreply.github.com>
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.
Thanks Ciaran 👍
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 adding some things i noticed outside the changes (might be more)
There's a DB error type (which isn't used?) that refers to linked entities being pinned by entityVersionId
hash/packages/hash/api/src/db/errors.ts
Line 80 in db90afc
invalid: { entityId: string; entityVersionId?: string }[]; |
The changes to implied history means that the comment here is not true anymore hash/packages/hash/api/src/db/postgres/history.ts Lines 75 to 79 in db90afc
And I assume we can remove the conditional parts here that deal with fixed VersionIds hash/packages/hash/api/src/db/postgres/history.ts Lines 238 to 255 in db90afc
I think this change might be best to do in a separate PR, though. |
…hash into cm/update-bp-package-versions
Thanks @thehabbos007 – removed the unused error and updated comments in dcd8d4e |
This pull request introduces 1 alert when merging c44707c into 4b72242 - view on LGTM.com new alerts:
|
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.
Thanks Ciaran! Looks good to me :)
🌟 What is the purpose of this PR?
Upgrades the versions we are using of the
blockprotocol
andmock-block-dock
packages, mainly to eliminate the patching we were doing related tolinkedAggregations
, following blockprotocol/blockprotocol#302Since the latest
blockprotocol
removes provision fordestinationEntityVersionId
, I have also removed our support for it (that we weren't using) from HASH.As part of this I actually made a few more changes to bp types locally, to split out the
Filter
related types further. I am going to defer reflecting these changes back into BP and then updating here again, as it can wait.🔗 Related links