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: improve error messaging #893

Merged
merged 3 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/live-preview-sdk/src/__tests__/init.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe('init', () => {

it('warns about invalid init call (missing locale)', () => {
expect(ContentfulLivePreview.init).toThrow(
"Init function have to be called with a locale configuration (for example: `ContentfulLivePreview.init({ locale: 'en-US'})`)",
"Init function has to be called with a locale configuration (for example: `ContentfulLivePreview.init({ locale: 'en-US'})`)",
);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/live-preview-sdk/src/__tests__/react.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('ContentfulLivePreviewProvider', () => {
// @ts-expect-error -- case locale not provided (e.g. JavaScript usage)
() => render(<ContentfulLivePreviewProvider>Hello World</ContentfulLivePreviewProvider>),
).toThrowError(
'ContentfulLivePreviewProvider have to be called with a locale property (for example: `<ContentfulLivePreviewProvider locale="en-US">{children}</ContentfulLivePreviewProvider>`',
'ContentfulLivePreviewProvider has to be called with a locale property (for example: `<ContentfulLivePreviewProvider locale="en-US">{children}</ContentfulLivePreviewProvider>`',
);

spy.mockRestore();
Expand Down
5 changes: 3 additions & 2 deletions packages/live-preview-sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export class ContentfulLivePreview {
static init(config: ContentfulLivePreviewInitConfig): Promise<InspectorMode | null> | undefined {
if (typeof config !== 'object' || !config?.locale) {
throw new Error(
"Init function have to be called with a locale configuration (for example: `ContentfulLivePreview.init({ locale: 'en-US'})`)",
"Init function has to be called with a locale configuration (for example: `ContentfulLivePreview.init({ locale: 'en-US'})`)",
);
}

Expand Down Expand Up @@ -345,6 +345,7 @@ export class ContentfulLivePreview {
return this.liveUpdatesEnabled;
}

//TODO: Rename to openEntityInEditor, as assets can also be opened. Would be breaking change
static openEntryInEditor(props: LivePreviewProps): void {
const defaultProps = {
locale: this.locale,
Expand Down Expand Up @@ -374,7 +375,7 @@ export class ContentfulLivePreview {
return;
}

debug.error('Please provide field id and entry id to openEntryInEditor.');
debug.error('Please provide field id and entry/asset id to openEntryInEditor.', { ...props });
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ describe('getAllTaggedElements', () => {

const elements = getAllTaggedElements({
root: dom,
ignoreManual: true,
options: { locale: 'en-US' },
options: { locale: 'en-US', ignoreManuallyTaggedElements: true },
});

expect(elements).toHaveLength(1 + 10 + 30 + 3);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { combine } from '@contentful/content-source-maps';
import { describe, expect, it } from 'vitest';
import { describe, expect, it, vi } from 'vitest';

import { setDebugMode } from '../../helpers/debug.js';
import { InspectorModeDataAttributes, InspectorModeEntryAttributes } from '../types.js';
import { getAllTaggedElements } from '../utils.js';
import { createSourceMapFixture } from './fixtures/contentSourceMap.js';
Expand Down Expand Up @@ -174,11 +175,15 @@ describe('getAllTaggedElements', () => {
`<span>${combine('Test', createSourceMapFixture('ignore_origin', { origin: 'example.com' }))}</span>`,
);

setDebugMode(true);
vi.spyOn(console, 'warn');

const { taggedElements: elements } = getAllTaggedElements({
root: dom,
options: { locale: 'en-US' },
});
expect(elements).toEqual([]);
expect(console.warn).toHaveBeenCalled();
});

it('should recognize auto-tagged elements', () => {
Expand Down Expand Up @@ -392,8 +397,7 @@ describe('getAllTaggedElements', () => {
manuallyTaggedCount,
} = getAllTaggedElements({
root: dom,
ignoreManual: true,
options: { locale: 'en-US' },
options: { locale: 'en-US', ignoreManuallyTaggedElements: true },
});

expect(elements.length).toEqual(1);
Expand Down
22 changes: 18 additions & 4 deletions packages/live-preview-sdk/src/inspectorMode/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,16 @@ export function getManualInspectorModeAttributes(
manuallyTagged: true,
};

if (!sharedProps.fieldId) {
debug.warn('Element is missing field ID attribute and cannot be tagged', {
id:
element.getAttribute(InspectorModeDataAttributes.ENTRY_ID) ??
element.getAttribute(InspectorModeDataAttributes.ASSET_ID),
sharedProps,
});
return null;
}

const entryId = element.getAttribute(InspectorModeDataAttributes.ENTRY_ID);
if (entryId) {
return { ...sharedProps, entryId };
Expand Down Expand Up @@ -148,18 +158,16 @@ function hasTaggedParent(node: HTMLElement, taggedElements: TaggedElement[]): bo
export function getAllTaggedElements({
root = window.document,
options,
ignoreManual,
}: {
root?: any;
options: Omit<InspectorModeOptions, 'targetOrigin'>;
ignoreManual?: boolean;
}): {
taggedElements: TaggedElement[];
manuallyTaggedCount: number;
automaticallyTaggedCount: number;
autoTaggedElements: AutoTaggedElement<Element>[];
} {
const alreadyTagged = ignoreManual
const alreadyTagged = options.ignoreManuallyTaggedElements
? []
: root.querySelectorAll(
`[${InspectorModeDataAttributes.ASSET_ID}][${InspectorModeDataAttributes.FIELD_ID}], [${InspectorModeDataAttributes.ENTRY_ID}][${InspectorModeDataAttributes.FIELD_ID}]`,
Expand All @@ -180,7 +188,13 @@ export function getAllTaggedElements({
for (const { node, text } of stegaNodes) {
const sourceMap = decode(text);
if (!sourceMap || !sourceMap.origin.includes('contentful.com')) {
debug.warn;
debug.warn(
"Element has missing or invalid ContentSourceMap, please check if you have correctly enabled ContentSourceMaps and that the element's data originates from Contentful",
Copy link
Contributor

@2wce 2wce Sep 18, 2024

Choose a reason for hiding this comment

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

@MayaGillilan are we trying to imply that the data shouldn't be transformed once it comes from our APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to admit I'm not as familiar with CSM so that was my understanding, but if that's an inaccurate description please let me know what would be more accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@2wce @YvesRijckaert Can you confirm if this is an appropriate message?

{
node,
sourceMap,
},
);
continue;
}

Expand Down
11 changes: 11 additions & 0 deletions packages/live-preview-sdk/src/liveUpdates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ export class LiveUpdates {

const subscription = this.subscriptions.get(subscriptionId);

if (!data.sys || !data.sys.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@MayaGillilan Just as an FYI: this broke live updates for GraphQL:
data could look like this for GraphQL:

{
    "postCollection": {
        "items": [
            {
                "__typename": "Post",
                "sys": {
                    "id": "a1b2c3"
                },
                "slug": "foo",
                "title": "new value​​​​‌‍​‍​‍‌‍‌​‍‌‍‍‌‌‍‌‌‍‍‌‌‍‍​‍​‍​‍‍​‍​‍‌‍​‌‍‌‍‍‌‌​‌‍‌‌‌‍‍‌‌​‌‍‌‍‌‌‌‌‍​​‍‍‌‍​‌‍‌‍‌​‍​‍​‍​​‍​‍‌‍‍​‌​‍‌‍‌‌‌‍‌‍​‍​‍​‍‍​‍​‍‌‍‍​‌‌​‌‌​‌​​‌​​‍‍​‍​‍‌‍​‌‌​​‌​​​‍‍‌‍​‌‍‌‍‍‌‌​‌‍‌‌‌‍‍‌‌​‌‍‌‍‌‌‌‌‍​​‍‍‌‍​‌‍‌‍‌​‍‌​‌​​‌‍​‌‌‍​‌‍‌‌‌​​‍‌‌​‌‍‌‌‌​‌‌​​‍‌‌​‌​​‌‍​‌‌‍​‌‍‌‌​‍‌‍‌‌‌‍‍‌‌‍‌‍‍‌‌​‍‌‍‌‍‍‌‍‌‌‍‌‌‌‍‍‌‌​‌​​‍‌‍‌‌‍​‌‌​‌‌​‌‍‌‌‌​‍​‍‌‍‌‌‌‍‍‌‌​‌​‍‌‍‍‌‌‍‌‌‌​​‍‌‍​‌​​‌‌‍​‍​​‍‌‍​​​​‍​‌‍‌‍‌‍‌‍​‌‌‌‌​‌‍‌‌‌‍‌​‌​‌‍‌‍‍‌‌‍‌‌‌‍​‌‍‌​​‌‌‌​‌‍‍‌‌‌​‌‍​‌‍‌‌​‍‌‍‌‍‌‍‌‍‌‍​‌‌‌‌​‌‍‌‌‌‍‌​‌​​‌‍‌‍​‌‍​‌‌‍​‌‍‌‌​‌‌‍‌‌‌‍‍​‍‌‌‌‌‌‌‌​​‍‌‍‌​‌‍‌‌‌‌​‍‌‍​‌‍‌‌​‌‌‌‍‌‍‌‌‌​‍‌‍​‌‍‌‌‌‍​​‍‌‌‍​‌‍‌‍‍‌‌​‌‍‌‌‌‍‍‌‌​​‍‌‌‍​‌‍‍‌‌‍‍‌‍‍​‍​‍​‍​​‍​‍‌‍​‌‍‌‍‍‌‌​‌‍‌‌‌‍‍‌‌​‌‍‌‍‌‌‌‌‍​​‍​‍​‍‍‌‍​‍​‍‌‍‌‌‌‍‌​‌‍‍‌‌‌​‌‍‌​‍‌​‍‌‌‍‍‌‌​‌‍‌‌‌​‍‌‍‌‍‌‍​‌‌‍​‌‍‌‌​‍​‍​‍‍‌‍​‍​‍‌‌‌‍‍‌‌‍‌​‌‍‌‌‍‌‌‌‌​‌​‍‌‍​‌‌‍‌‌‍‌‌‌​‌​​‌‍​‌‌‍​‌‍‌‌​‍​‍​‍‍​‍​‍‌‍​‍‌‌‌‌‍‍‌‌‍​‌‌​‌‍‍‌‌‍‍​‍​‍​‍​​‍​‍‌‌‌‍‍‌‌‍‌​‌‍‌‌‍‌‌‌‌​‌​‍‌‌‍‌​​‍​‍​‍‍​‍​‍‌​‌‍‍‌‌‍‍‌‍‌‌‍​‌‍‌‌‌​​‌‍‍‌‌‍‍‌‍‌‌​‍​‍‌‌​‍​​‍​‍‌‍‌‍‌‍‍‌‌‍‌‌‌‍​‌‍‌​‌‌‌​‌‍‌‌​​‌‍‌‌​‍​‍​‍‍​‍​‍‌‌​‌‍‌‌‍‌‌‍​‍‌‍‌‍​​‍​‍‌‌‌‌",
                ]
            }
        ]
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

debug.error(
'Received an update with missing `sys.id`, please provide `sys.id` in order to enable live updates',
{
data,
subscriptionId,
},
);
return;
}

if (subscription) {
subscription.callback(data);
subscription.data = data;
Expand Down
2 changes: 1 addition & 1 deletion packages/live-preview-sdk/src/react.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export function ContentfulLivePreviewProvider({
}: PropsWithChildren<ContentfulLivePreviewInitConfig>): ReactElement {
if (!locale) {
throw new Error(
'ContentfulLivePreviewProvider have to be called with a locale property (for example: `<ContentfulLivePreviewProvider locale="en-US">{children}</ContentfulLivePreviewProvider>`',
'ContentfulLivePreviewProvider has to be called with a locale property (for example: `<ContentfulLivePreviewProvider locale="en-US">{children}</ContentfulLivePreviewProvider>`',
);
}

Expand Down
Loading