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

chore: maintain package #63

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

chore: maintain package #63

wants to merge 6 commits into from

Conversation

lihbr
Copy link
Member

@lihbr lihbr commented May 23, 2023

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

See commit history~

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.
🐢

@lihbr lihbr requested a review from angeloashmore May 23, 2023 10:04
Copy link
Member

@angeloashmore angeloashmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I just left one small style comment for size-limit.cjs.

.size-limit.cjs Show resolved Hide resolved
Copy link
Member Author

@lihbr lihbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for updating it

},
"dependencies": {
"@prismicio/types": "^0.2.7"
"@prismicio/client": "^7.1.1"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this install well? 🤔 Wondering if NPM could not be confused by the recursive dependency. I guess we'll discover it soon enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eek, it doesn't. I was worried about that before as well (Slack thread), and unfortunately it seems there is an issue.

Here's the error I see after installing @prismicio/richtext into nextjs-starter-prismic-multi-page:

[next] - error Failed to load next.config.js, see more info here https://nextjs.org/docs/messages/next-config-error
[next] (node:24766) Warning: Accessing non-existent property 'RichTextNodeType' of module exports inside circular dependency
[next] (Use `node --trace-warnings ...` to show where the warning was created)
[next] /Users/angeloashmore/projects/prismic/nextjs-starter-prismic-multi-page/node_modules/@prismicio/richtext/dist/types.cjs:5
[next]   [client.RichTextNodeType.listItem]: "listItem",
[next]                            ^
[next]
[next] TypeError: Cannot read properties of undefined (reading 'listItem')
[next]     at Module.<anonymous> (/Users/angeloashmore/projects/prismic/nextjs-starter-prismic-multi-page/node_modules/@prismicio/richtext/dist/types.cjs:5:28)
[next]     at Module._compile (node:internal/modules/cjs/loader:1254:14)
[next]     at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
[next]     at Module.load (node:internal/modules/cjs/loader:1117:32)
[next]     at Module._load (node:internal/modules/cjs/loader:958:12)
[next]     at Module.require (node:internal/modules/cjs/loader:1141:19)
[next]     at require (node:internal/modules/cjs/helpers:110:18)
[next]     at Module.<anonymous> (/Users/angeloashmore/projects/prismic/nextjs-starter-prismic-multi-page/node_modules/@prismicio/richtext/dist/wrapMapSerializer.cjs:3:15)
[next]     at Module._compile (node:internal/modules/cjs/loader:1254:14)
[next]     at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
[next]
[next] Node.js v18.16.0
[next] npm run next:dev exited with code 1

We could either:

  1. Move rich text-specific types into @prismicio/richtext, removing the dependency on @prismicio/client.
  2. Merge @prismicio/richtext into @prismicio/client, resulting in a breaking change, but simplifying the ecosystem.

#1 seems like the better and simpler solution, but what do you think?

Copy link
Member Author

@lihbr lihbr Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with 1.

I considered also 3. which would have been about inlining @prismicio/client types in @prismicio/richtext (at build time), but this might cause more issues in the long run as we improve/fix types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR implementing idea 2 (merge @prismicio/richtext into @prismicio/client) after attempting idea 1 (move rich text-specific types from @prismicio/client into @prismicio/richtext).

Pull request: prismicio/prismic-client#318

Idea 1 didn't work well since many rich text node types depend on field types which only live in @prismicio/client.

While we could copy the types over or inline @prismicio/client, we could eventually run into type incompatibilities and implementations change. Merging @prismicio/richtext into @prismicio/client allows us to change implementations without mismatching types.

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.

2 participants