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

fix: resolve previewed document URL for any language #208

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

AriFreyr
Copy link
Contributor

Types of changes

  • Chore (a non-breaking change which is related to package maintenance)
  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Preview only works for the default language, fetch the preview document for all languages. Otherweise getByID will crash before calling the linkResolver

Checklist:

  • My change requires an update to the official documentation.
  • All TSDoc comments are up-to-date and new ones have been added where necessary.
  • All new and existing tests are passing.

@angeloashmore
Copy link
Member

Hi @AriFreyr, thanks for the PR!

This is a good catch. I wonder if this could cause problems in other parts of an app once it gets past getByID(). If you preview a document in language "B", for example, but the rest of app uses language "A", you may be mixing documents from other languages.

I can see how your change is necessary, even in that situation. If an app uses the URL to determine how the client is configured, for example, you would need to first get past resolvePreviewURL() for the document.

Requested changes

Could you update the tests for resolvePreviewURL() in test/client-resolvePreviewURL.test.ts?

In each test, the following lang: "*" line should be added in the createMockQueryHandler() function:

  server.use(
    createMockRepositoryHandler(t),
    createMockQueryHandler(t, [queryResponse], undefined, {
      ref: previewToken,
      q: `[[at(document.id, "${documentId}")]]`,
+     lang: "*",
    }),
  );

I can also make these changes if you enable maintainer edits (details here).

Thanks!

@AriFreyr
Copy link
Contributor Author

AriFreyr commented Jan 15, 2022

This is a good catch. I wonder if this could cause problems in other parts of an app once it gets past getByID(). If you preview a document in language "B", for example, but the rest of app uses language "A", you may be mixing documents from other languages.

Yeah i can see how it would be confusing, i think the only other way to address this (and maybe a better way, atleast for my use case) would be if the preview callback url provided the language of the document as a query param.

I fixed the tests, should be working now, also edits should be allowed so feel free to edit

@angeloashmore
Copy link
Member

Great! Thanks for updating the tests. I'll merge and publish this. 🙂

I think the only other way to address this (and maybe a better way, atleast for my use case) would be if the preview callback url provided the language of the document as a query param.

I could see that being useful. Since the previewed document contains the language in the lang parameter, the language should be obtainable easily, albeit with a network request.

@codecov-commenter
Copy link

Codecov Report

Merging #208 (b67e3ec) into master (a883f7e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #208   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        19           
  Lines          316       316           
  Branches        60        60           
=========================================
  Hits           316       316           
Impacted Files Coverage Δ
src/client.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a883f7e...b67e3ec. Read the comment docs.

@angeloashmore angeloashmore changed the title fetch preview document for all languages fix: fetch preview document for any languages Jan 18, 2022
@angeloashmore angeloashmore changed the title fix: fetch preview document for any languages fix: resolve previewed document URL any language Jan 18, 2022
@angeloashmore angeloashmore merged commit a7504be into prismicio:master Jan 18, 2022
@angeloashmore angeloashmore changed the title fix: resolve previewed document URL any language fix: resolve previewed document URL for any language Jan 18, 2022
@angeloashmore
Copy link
Member

This has been published in v6.1.0.

Thanks again @AriFreyr!

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

Successfully merging this pull request may close these issues.

3 participants