-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: add createMigration
and createWriteClient
#350
Conversation
Co-authored-by: Angelo Ashmore <angeloashmore@users.noreply.github.com>
Thank you so much for this first batch of reviews! I addressed them all and replied where necessary. Let's have a quick sync chat tonight to close some topics if you're available 👌 |
Co-authored-by: Angelo Ashmore <angeloashmore@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.
New comments for the tests! Addressing these comments are less important than the ones made on the implementation.
alt: "foo", | ||
credits: "bar", | ||
notes: "baz", |
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.
alt
, credits
, and notes
? Otherwise, we don't know if the data was passed through.
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.
The issue I had here is that it's passed as a FormData so I cannot read it on the mocked API side unless we manage to upgrade MSW (out of scope here).
I'll add a TODO comment to rework this test when we update MSW.
refactor: use classes to detect migration field
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.
This is excellent. The latest version is simpler and might be less prone to bugs.
I left a few comments, none of which are blocking.
Thank you so much! Addressed the last comments, should be good to go now 🎉 I'll hold off on merging until we're ready on the documentation end of things. |
Implements: https://www.notion.so/prismic/RFC-Migration-API-Client-7365f2cce2ac43178955287dbad82e9c
Description
This Pull Request implements a new
WriteClient
allowing users to communicate with Prismic write APIs, namely the Migration API with ease, thanks to a helperMigration
class working in concert with the new client.Reworked structure
Client
's fetching logic has been abstracted in a new abstractBaseClient
class, shaving off some ~300 lines from theClient
class itself.createClient.ts
file has been split intocreateClient.ts
containing only the factory, andClient.ts
containing the class itself.WriteClient
andMigration
classes.WriteClient
extendsClient
which itself extendsBaseClient
, allowing for better tree shakability.Reworked constructor
Client
's constructor has been updated to favor arepositoryName
to be used as the first argument when creating a client. It now emits warnings when therepositoyName
cannot be inferred.WriteClient
's constructor will also warn when running in a browser-like environment to let users know that they credentials might be leaking.New types
src/types/api/asset
andsrc/types/api/migration
respectively.src/types/migration
New internal helpers
lib
) have been added to support the newWriteClient
, namelypatchMigrationDocumentData
which contains all the crawling logic for correctly patching fields from a migration document with final field values and their updated IDs.New exports
WriteClient
has two exposed methods,migrate()
andgetAssets()
, the other ones are kept private. It is exported alongside its factorycreateWriteClient
WriteClient
'smigrate()
methods supports areporter
option reporting various events happening during the migration process. Thoses event types are exported asMigrateReporterEvents
Migration
is exported alongside its factorycreateMigration
Tests
createRepositoryName
to ensure repository names are unique for each tests.Migration
methods are fully tested:createAsset()
createDocument()
getByUID()
getSingle()
WriteClient
public methods are fully tested:migrate()
testMigrationFieldPatching
getAssets()
WriteClient
private methods, cheering my way through TypeScript:createAsset()
createAssetTag()
fetchForeignAsset()
createDocument()
updateDocument()
AbortController
on most of them, I left a comment pointing to the suspected upstream issue related to that one, but I don't really get what's going wrong and where because other tests not going throughp-limit
are fine 🤔Todos
createMigration
andcreateWriteClient
factory types.@prismicio/client
'sWriteClient
prismic-ts-codegen#65Checklist
How to QA 1
Use the following zip as a starting point: https://prismic-team.slack.com/files/U014KACFM6K/F07LY4GDLRE/migration-client-demo.zip
Following the
README.md
gives you a basic flow.Footnotes
Please use these labels when submitting a review:
⚠️ #issue: Strongly suggest a change.
❓ #ask: Ask a question.
💡 #idea: Suggest an idea.
🎉 #nice: Share a compliment. ↩