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.

Sign up for GitHub

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

Jump to bottom

feat: allow document parameter of getRichTextEntityLinks to be nullable [] #471

Merged
merged 1 commit into from
Jul 10, 2023
Merged

feat: allow document parameter of getRichTextEntityLinks to be nullable [] #471

merged 1 commit into from
Jul 10, 2023

Conversation

BasKiers
Copy link
Contributor

@BasKiers BasKiers commented Jun 7, 2023

As an immediate action of this postmortem we decided to let the public type reflect the expected type that is passed to the function.

This means that the type is now less strict which will make it clearer why we deliberately check for null values in our library code. This should cause this exact issue to not happen again (by accident) in the future.

@BasKiers BasKiers requested a review from anho June 7, 2023 14:44
@BasKiers BasKiers force-pushed the feat/allow-nullable-values-as-parameter branch 2 times, most recently from ae0cb38 to 2c8c6fd Compare June 7, 2023 14:49
@contentful-automation
Copy link
Contributor

contentful-automation bot commented Jun 7, 2023 •
edited
Loading

Size Change: +164 B (0%)

Total Size: 255 kB

ℹ️ View Unchanged
Filename Size Change
./packages/contentful-slatejs-adapter/dist/contentful-slatejs-adapter.es5.js 2.4 kB 0 B
./packages/contentful-slatejs-adapter/dist/lib/__test__/contentful-helpers.js 344 B 0 B
./packages/contentful-slatejs-adapter/dist/lib/__test__/contentful-to-slatejs-adapter.test.js 2.09 kB 0 B
./packages/contentful-slatejs-adapter/dist/lib/contentful-to-slatejs-adapter.js 858 B 0 B
./packages/contentful-slatejs-adapter/dist/lib/helpers.js 157 B 0 B
./packages/contentful-slatejs-adapter/dist/lib/index.js 143 B 0 B
./packages/contentful-slatejs-adapter/dist/lib/schema.js 536 B 0 B
./packages/contentful-slatejs-adapter/dist/lib/slatejs-to-contentful-adapter.js 1.01 kB 0 B
./packages/contentful-slatejs-adapter/dist/lib/types/index.js 64 B 0 B
./packages/contentful-slatejs-adapter/dist/lib/types/slate.js 67 B 0 B
./packages/rich-text-from-markdown/dist/lib/__test__/helpers.js 640 B 0 B
./packages/rich-text-from-markdown/dist/lib/__test__/index.test.js 2.35 kB 0 B
./packages/rich-text-from-markdown/dist/lib/__test__/real-world.test.js 2.58 kB 0 B
./packages/rich-text-from-markdown/dist/lib/index.js 3.83 kB 0 B
./packages/rich-text-from-markdown/dist/lib/types/index.js 125 B 0 B
./packages/rich-text-from-markdown/dist/rich-text-from-es5.js 191 kB 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/embedded-entry.js 310 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/embedded-js 318 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/heading.js 322 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/hr.js 257 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/hyperlink.js 309 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/index.js 594 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/inline-entity.js 376 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/invalid-marks.js 282 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/invalid-type.js 312 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/mark.js 278 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/ol.js 321 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/paragraph.js 244 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/quote.js 257 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/table-header.js 410 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/table.js 399 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/documents/ul.js 322 B 0 B
./packages/rich-text-html-renderer/dist/lib/__test__/index.test.js 1.93 kB 0 B
./packages/rich-text-html-renderer/dist/lib/index.js 1.57 kB 0 B
./packages/rich-text-html-renderer/dist/rich-text-html-es5.js 4.94 kB 0 B
./packages/rich-text-links/dist/lib/__test__/index.test.js 1.93 kB +40 B (+2%)
./packages/rich-text-links/dist/lib/index.js 1.49 kB 0 B
./packages/rich-text-links/dist/rich-text-links.es5.js 1.5 kB 0 B
./packages/rich-text-plain-text-renderer/dist/lib/__test__/index.test.js 960 B 0 B
./packages/rich-text-plain-text-renderer/dist/lib/index.js 1.27 kB 0 B
./packages/rich-text-plain-text-renderer/dist/rich-text-plain-text-es5.js 3.89 kB 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/components/js 287 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/components/Paragraph.js 287 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/components/Strong.js 287 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/embedded-entry.js 310 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/embedded-js 318 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/heading.js 322 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/hr.js 257 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/hyperlink.js 309 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/index.js 661 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/inline-entity.js 376 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/invalid-marks.js 282 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/invalid-type.js 312 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/mark.js 278 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/multi-mark.js 290 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/ol.js 321 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/paragraph.js 244 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/quote.js 257 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/table-header.js 410 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/table.js 399 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/documents/ul.js 322 B 0 B
./packages/rich-text-react-renderer/dist/lib/__test__/index.test.js 2.55 kB 0 B
./packages/rich-text-react-renderer/dist/lib/index.js 1.18 kB 0 B
./packages/rich-text-react-renderer/dist/lib/util/appendKeyToValidElement.js 267 B 0 B
./packages/rich-text-react-renderer/dist/lib/util/nodeListToReactComponents.js 599 B 0 B
./packages/rich-text-react-renderer/dist/rich-text-react-es5.js 4.44 kB 0 B
./packages/rich-text-types/dist/__test__/index.test.js 587 B 0 B
./packages/rich-text-types/dist/__test__/schemaConstraints.test.js 564 B 0 B
./packages/rich-text-types/dist/blocks.js 492 B 0 B
./packages/rich-text-types/dist/emptyDocument.js 361 B 0 B
./packages/rich-text-types/dist/helpers.js 489 B 0 B
./packages/rich-text-types/dist/index.js 792 B 0 B
./packages/rich-text-types/dist/inlines.js 348 B 0 B
./packages/rich-text-types/dist/marks.js 291 B 0 B
./packages/rich-text-types/dist/nodeTypes.js 127 B 0 B
./packages/rich-text-types/dist/schemaConstraints.js 1.13 kB 0 B
./packages/rich-text-types/dist/schemas/__test__/helpers.test.js 371 B 0 B
./packages/rich-text-types/dist/schemas/__test__/schemas.test.js 425 B 0 B
./packages/rich-text-types/dist/schemas/index.js 274 B 0 B
./packages/rich-text-types/dist/types.js 124 B 0 B
./packages/rich-text-links/dist/lib/types/utils.js 124 B +124 B (new file) 🆕

@anho anho requested review from DanweDE and a team June 8, 2023 13:40
aodhagan-cf
aodhagan-cf reviewed Jun 8, 2023
View reviewed changes
packages/rich-text-links/src/index.ts
@@ -82,7 +83,7 @@ function isContentNode(node: Node): node is Inline | Block {
return 'content' in node && Array.isArray(node.content);
}

function visitNodes(startNode: Node, onVisit: (node: Node) => void): void {
function visitNodes(startNode: Maybe<Node>, onVisit: (node: Node) => void): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we don't need the maybe on onVisit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any nullable values are handled on line 90 and will not be passed along to onVisit. This prevents consumers of this method to add additional null checks their handler functions

@BasKiers BasKiers force-pushed the feat/allow-nullable-values-as-parameter branch from 2c8c6fd to f9b1711 Compare July 10, 2023 13:50
@BasKiers BasKiers enabled auto-merge (squash) July 10, 2023 13:51
@BasKiers BasKiers merged commit e50f8d2 into master Jul 10, 2023
5 checks passed
@BasKiers BasKiers deleted the feat/allow-nullable-values-as-parameter branch July 10, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aodhagan-cf aodhagan-cf aodhagan-cf left review comments

@DanweDE DanweDE DanweDE approved these changes

@anho anho Awaiting requested review from anho

Assignees
No one assigned
Labels
package:rich-text-links
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BasKiers @DanweDE @aodhagan-cf