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

feat: use @prismicio/client v6 internally and update dependencies #40

Merged
merged 17 commits into from
Feb 28, 2022

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented Feb 15, 2022

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

This PR re-implements apollo-link-prismic using @prismicio/client v6's graphqlFetch() method internally. This simplifies the Link greatly.

It also updates all dependencies to the most up-to-date versions.

Lastly, it updates the repository to use the latest Prismic TypeScript project standards.

Fixes: #38

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.

👩‍🚀

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@e95da4d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             master       #40   +/-   ##
==========================================
  Coverage          ?   100.00%           
==========================================
  Files             ?         2           
  Lines             ?        16           
  Branches          ?         3           
==========================================
  Hits              ?        16           
  Misses            ?         0           
  Partials          ?         0           

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 e95da4d...1419fb5. Read the comment docs.

@angeloashmore
Copy link
Member Author

angeloashmore commented Feb 15, 2022

⚠️ TODO: Re-enable Size Limit in .github/workflows/ci.yml after merging into master.

@bencehornak
Copy link

@angeloashmore it integrated well into our project.

Just one more thing: although the tsdoc comment mentions that the fetch parameter is needed on Node, I would consider highlighting this in the README too with a Node specific example:

import { ApolloClient, InMemoryCache } from "@apollo/client";
import { createPrismicLink } from "apollo-link-prismic";
import fetch from "node-fetch";

const apolloClient = new ApolloClient({
	link: createPrismicLink({
		repositoryName: "YOUR_REPOSITORY_NAME",
		// Provide your access token if your repository is secured.
		accessToken: "YOUR_ACCESS_TOKEN",
		fetch: fetch,
	}),
	cache: new InMemoryCache(),
});

@angeloashmore
Copy link
Member Author

@bencehornak Thanks for trying it out!

Good idea about documenting the need to pass a fetch implementation. The official documentation (https://prismic.io/docs/technologies/integrate-with-existing-js-project-graphql) will be updated soon to reflect the changes made here. You'll see the docs are outdated (for example, they don't use @apollo/client). I'll be sure to note that passing fetch is required in fetch-less environments.

Until those docs are updated, I'll update the README like you suggest.

@angeloashmore
Copy link
Member Author

@bencehornak [email protected] was just published with the type fix. Could you confirm that this matches what you expect?

If it does, I'll move forward with publishing this as latest. Thanks!

@bencehornak
Copy link

@angeloashmore now the typing works great as well!

One final thought: since this PR brings in breaking changes (e.g. now the fetch argument is required in some environments), wouldn't it be better to bump the major version? (semver #8)

@angeloashmore
Copy link
Member Author

angeloashmore commented Feb 17, 2022

@bencehornak Correct me if your experience is different, but the current version also requires fetch to be provided by the environment or the fetch option.

It isn't documented in the current README or in the code. It uses Apollo's HttpLink internally (as does the updated version), which needs a global or provided fetch function (see this section of the Apollo docs).

So the API should be the same, but it is now explicitly documented in the form of TSDocs and the README (and eventually in the official Prismic docs).

@angeloashmore angeloashmore changed the title feat: use @prismicio/client v6 internally along with the most up-to-date dependencies feat: use @prismicio/client v6 internally and update dependencies Feb 17, 2022
@bencehornak
Copy link

@angeloashmore you must be right.

(The reason, why I was confused, is that I actually had to add the fetch property now. It worked so far without it, because of the presence of a polyfill, provided by some of our dependencies. @prismicio/client for some reason didn't pick up the polyfill, so I had to provide fetch explicitly. I don't think this is a bug with @prismicio/client, it's rather a problem of the dependency, which loaded the polyfill in a non-standard way.)

Anyway, I think this sounds like an edge-case, most users shouldn't experience a breaking change here, so the minor version bump should be fine.

@angeloashmore angeloashmore merged commit 9639d2b into master Feb 28, 2022
@angeloashmore angeloashmore deleted the aa/refresh branch February 28, 2022 19:32
@angeloashmore
Copy link
Member Author

Thanks for testing it out @bencehornak.

This PR is published in v1.1.0 🎉

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.

Upgrade vulnerable dependency
3 participants