chore(tasks): move CommentsSetupProvider to core #5767
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
pedrobonamin
force-pushed
the
comments-setup
branch
from
d2b2eb7
to
5c27f97
Compare
pedrobonamin
force-pushed
the
comments-setup
branch
from
5c27f97
to
faf2043
Compare
Full Reportsanity
@sanity/migrate
@sanity/diff
@sanity/block-tools
@sanity/types
sanity/desk
@sanity/portable-text-editor
@sanity/mutator
@sanity/cli
sanity/structure
@sanity/util/concurrency-limiter
@sanity/util/legacyDateFormat
@sanity/schema/_internal
@sanity/util/paths
sanity/router
@sanity/schema
sanity/cli
@sanity/vision
@sanity/util/fs
sanity/_internal
@sanity/util/client
@sanity/util/createSafeJsonParser
sanity/_internalBrowser
@sanity/util/content
|
|
Component Testing Report Updated Feb 23, 2024 2:47 PM (UTC)
|
pedrobonamin
requested review from
skogsmaskin,
cngonzalez and
hermanwikner
and removed request for
a team
packages/sanity/src/core/studio/commentsSetup/CommentsSetupProvider.tsx
Outdated
Show resolved
Hide resolved
pedrobonamin
force-pushed
the
comments-setup
branch
from
0e6d8e6
to
f9db0da
Compare
| setIsRunningSetup(false) | ||
| return client | ||
| } | ||
| } catch (_) { |
There was a problem hiding this comment.
I'm afraid I'm not as close to this as you are, but is there a specific error we can catch here? I'm just concerned about valid errors being ignored and further work being done, getting into a strange state.
The additional error catching below might suffice, but was curious if we could get more specificity here.
There was a problem hiding this comment.
Thanks @cngonzalez will defer to @hermanwikner and @sjelfull on this.
There was a problem hiding this comment.
Also realize that this is rearranging pre-exsiting code so maybe better to address in another PR 😅
There was a problem hiding this comment.
I did some testing, and it appears we no longer need this check. The logic in this try/catch was initially added because the /setup endpoint used to return an error if the addon dataset had already been created. However, this no longer seems to be the case, as the endpoint now returns the name of the addon dataset if it has already been created instead – which is great!
There was a problem hiding this comment.
The comment below mentions that it could return an error also if the dataset doesn't exists. Maybe we can do a follow up to consider if we remove it or not, wouldn't feel confortable removing it in this one given it should be mostly focused on re-arranging the changes and not refactors.
But also tried it and it doesn't fail if the dataset doesn't exists.
cngonzalez
dismissed
their stale review
would like to check with the team! sorry for noise
pedrobonamin
force-pushed
the
comments-setup
branch
from
f9db0da
to
c456956
Compare
pedrobonamin
force-pushed
the
comments-setup
branch
from
c456956
to
f8ccc61
Compare
pedrobonamin
force-pushed
the
comments-setup
branch
from
fde32dc
to
0934cce
Compare
pedrobonamin
force-pushed
the
comments-setup
branch
from
0934cce
to
5b65ebc
Compare
There was a problem hiding this comment.
Nice stuff – thanks @pedrobonamin 👍
I added one comment about renaming isRunningSetup to isCreatingDataset – but no blocker for merge!
Description
This PR moves the
<CommentsSetupProvider>from comments plugin to core.We need to move this provider because
tasksand upcoming features will need to use the comments (addon) dataset to store data.In favour of sharing code, this provider is moved to core.
The
runSetupfunction has been refactored and rename tocreateAddonDatasetto create the client but not create the comment / payload received, allowing for it's reusability.Naming wise, we are keeping it has been renamed from
CommentsSetupProvidertoAddonDatasetProvider.Main changes
File moved to core.
Function updated to only create the dataset.
Caller now creates the document.
What to review
Is the location of the new provider correct?
Testing
Creating a new comments dataset should work.
Notes for release
Moves the CommentSetupProvider into core for reusability.