Invalid typing for the sitecore react <Link> tag

jantimon commented 6 years ago

Description

The definitions of @sitecore-jss/sitecore-jss-react/types/components/Link.d.ts don't match the current boilerplate code.

Version "@sitecore-jss/sitecore-jss-react": "^9.0.5"

Expected behavior

Steps To Reproduce

1. Use jss to create a sitecore jss react app

2. Install the react type definitions using `npm i --save @types/react`

3. Create a tsconfig file using `npx tsc --init` and adjust the values accordingly

Click here to expand the tsconfig.json code

```ts { "compilerOptions": { /* Basic Options */ "target": "es5", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */ "module": "ESNext", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ "lib": ["dom", "es2017"], /* Specify library files to be included in the compilation. */ "allowJs": true, /* Allow javascript files to be compiled. */ // "checkJs": true, /* Report errors in .js files. */ "jsx": "react", /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */ // "declaration": true, /* Generates corresponding '.d.ts' file. */ // "declarationMap": true, /* Generates a sourcemap for each corresponding '.d.ts' file. */ // "sourceMap": true, /* Generates corresponding '.map' file. */ // "outFile": "./", /* Concatenate and emit output to single file. */ // "outDir": "./", /* Redirect output structure to the directory. */ // "rootDir": "./", /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */ // "composite": true, /* Enable project compilation */ // "removeComments": true, /* Do not emit comments to output. */ // "noEmit": true, /* Do not emit outputs. */ // "importHelpers": true, /* Import emit helpers from 'tslib'. */ // "downlevelIteration": true, /* Provide full support for iterables in 'for-of', spread, and destructuring when targeting 'ES5' or 'ES3'. */ // "isolatedModules": true, /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */ /* Strict Type-Checking Options */ "strict": true, /* Enable all strict type-checking options. */ "noImplicitAny": false, /* Raise error on expressions and declarations with an implied 'any' type. */ // "strictNullChecks": true, /* Enable strict null checks. */ // "strictFunctionTypes": true, /* Enable strict checking of function types. */ // "strictPropertyInitialization": true, /* Enable strict checking of property initialization in classes. */ // "noImplicitThis": true, /* Raise error on 'this' expressions with an implied 'any' type. */ // "alwaysStrict": true, /* Parse in strict mode and emit "use strict" for each source file. */ /* Additional Checks */ // "noUnusedLocals": true, /* Report errors on unused locals. */ // "noUnusedParameters": true, /* Report errors on unused parameters. */ // "noImplicitReturns": true, /* Report error when not all code paths in function return a value. */ // "noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */ /* Module Resolution Options */ "moduleResolution": "node", /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */ // "baseUrl": "./", /* Base directory to resolve non-absolute module names. */ // "paths": {}, /* A series of entries which re-map imports to lookup locations relative to the 'baseUrl'. */ // "rootDirs": [], /* List of root folders whose combined content represents the structure of the project at runtime. */ // "typeRoots": [], /* List of folders to include type definitions from. */ // "types": [], /* Type declaration files to be included in compilation. */ // "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */ "esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */ // "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */ /* Source Map Options */ // "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */ // "mapRoot": "", /* Specify the location where debugger should locate map files instead of generated locations. */ // "inlineSourceMap": true, /* Emit a single file with source maps instead of having a separate file. */ // "inlineSources": true, /* Emit the source alongside the sourcemaps within a single file; requires '--inlineSourceMap' or '--sourceMap' to be set. */ /* Experimental Options */ // "experimentalDecorators": true, /* Enables experimental support for ES7 decorators. */ // "emitDecoratorMetadata": true, /* Enables experimental support for emitting type metadata for decorators. */ } } ```

4. Rename `src/components/Styleguide-FieldUsage-Link/index.js` `to src/components/Styleguide-FieldUsage-Link/index.tsx`

5. View the file in VisualStudioCode

Possible Fix

Set the children attribute of LinkProps to React.ReactNode solves the issue.

For reference ReactNode allows the following values:

type ReactNode = ReactChild | ReactFragment | ReactPortal | boolean | null | undefined;

export interface LinkProps {
    /** The link field data. */
    field: LinkField | LinkFieldValue;
    /**
     * Can be used to explicitly disable inline editing.
     * If true and `field.editable` has a value, then `field.editable` will be processed and rendered as component output. If false, `field.editable` value will be ignored and not rendered.
     * @default true
     */
    editable?: boolean;
    /**
     * Displays a link text ('description' in Sitecore) even when children exist
     * NOTE: when in Sitecore Experience Editor, this setting is ignored due to technical limitations, and the description is always rendered.
     */
    showLinkTextWithChildrenPresent?: boolean;
    /**
     * React children will be rendered inside the <a> tag
     */
    children?: React.ReactNode;
    /** HTML attributes that will be appended to the rendered <a /> tag. */
    [attributeName: string]: any;
}

kamsar commented 5 years ago

Heh sorry for the apparent close without comment, but it was for the happy reason that it was closed via a commit that will fix this issue for GA. Thanks for the detailed outline of the issue :)

jantimon commented 5 years ago

Cool thanks 👍

I did some further research and found out two more things.

showLinkTextWithChildrenPresent?: boolean; does not match the boilerplate code showLinkTextWithChildrenPresent="true" as "true" is a string.
[attributeName: string]: any; is not a good idea because you lose all typings of attributes. A slightly better solution would be:

type LinkProps = React.DetailedHTMLProps<React.AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement> & {
/** The link field data. */
    field: LinkField | LinkFieldValue;
    /**
     * Can be used to explicitly disable inline editing.
     * If true and `field.editable` has a value, then `field.editable` will be processed and rendered as component output. If false, `field.editable` value will be ignored and not rendered.
     * @default true
     */
    editable?: boolean;
    /**
     * Displays a link text ('description' in Sitecore) even when children exist
     * NOTE: when in Sitecore Experience Editor, this setting is ignored due to technical limitations, and the description is always rendered.
     */
    showLinkTextWithChildrenPresent?: boolean;
}

That way you inherit all types of a regular a tag and therefore you don't have to set a type for children.

Furthermore you get full intellisense for all valid a tag attributes:

kamsar commented 5 years ago

That does look nice, but does it support custom attributes? (i.e. data-foo)

jantimon commented 5 years ago

Yes it does.

It supports aria and data attributes and string values like described in the react specs: https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html#data-and-aria-attributes

types

I prepared a demo to showcase that behaviour (click at App.tsx): https://codesandbox.io/s/8n6qnq12ql
(Please note that I removed the field type for the demo)

kamsar commented 5 years ago

Excellent. Merged. Also added some typings for srcset on the image component while I was at it - unfortunately we can't make that use image attribute typings because srcSet is already defined as a string and that doesn't match the component's needs.

kamsar commented 5 years ago

New Link.d.ts:

import React from 'react';
export interface LinkFieldValue {
    href?: string;
    className?: string;
    title?: string;
    target?: string;
    [attributeName: string]: any;
}
export interface LinkField {
    value: LinkFieldValue;
    editableFirstPart?: string;
    editableLastPart?: string;
}
export declare type LinkProps = React.DetailedHTMLProps<React.AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement> & {
    /** The link field data. */
    field: LinkField | LinkFieldValue;
    /**
     * Can be used to explicitly disable inline editing.
     * If true and `field.editable` has a value, then `field.editable` will be processed and rendered as component output. If false, `field.editable` value will be ignored and not rendered.
     * @default true
     */
    editable?: boolean;
    /**
     * Displays a link text ('description' in Sitecore) even when children exist
     * NOTE: when in Sitecore Experience Editor, this setting is ignored due to technical limitations, and the description is always rendered.
     */
    showLinkTextWithChildrenPresent?: boolean;
};
export declare const Link: React.SFC<LinkProps>;

New Image.d.ts:

import React from 'react';
export interface ImageFieldValue {
    src?: string;
    /** HTML attributes that will be appended to the rendered <img /> tag. */
    [attributeName: string]: any;
}
export interface ImageField {
    value?: ImageFieldValue;
    editable?: string;
}
export interface ImageSizeParameters {
    /** Fixed width of the image */
    w?: number;
    /** Fixed height of the image */
    h?: number;
    /** Max width of the image */
    mw?: number;
    /** Max height of the image */
    mh?: number;
    /** Ignore aspect ratio */
    iar?: 1 | 0;
    /** Allow stretch */
    as?: 1 | 0;
    /** Image scale. Defaults to 1.0 */
    sc?: number;
}
export interface ImageProps {
    /**
     * The image field data.
     * @deprecated use field property instead
     */
    media?: ImageField | ImageFieldValue;
    /** Image field data (consistent with other field types) */
    field?: ImageField | ImageFieldValue;
    /**
     * Can be used to explicitly disable inline editing.
     * If true and `media.editable` has a value, then `media.editable` will be processed
     * and rendered as component output. If false, `media.editable` value will be ignored and not rendered.
     */
    editable?: boolean;
    /**
     * Parameters that will be attached to Sitecore media URLs
     */
    imageParams?: {
        [paramName: string]: string | number;
    };
    srcSet?: Array<ImageSizeParameters>;
    /** HTML attributes that will be appended to the rendered <img /> tag. */
    [attributeName: string]: any;
}
export declare const Image: React.SFC<ImageProps>;

jantimon commented 5 years ago

Wow thank you so much for your fast reply and adopting those changes!! 👍

I took a look into your srcSet problem but I don't know if I understood you correctly.

If you have sth like type x = y & z it's a little bit like const x = Object.assign({}, y, z).
The last type will overwrite previous types.

So for the image if you take the following ImageProps example it would not allow to set a srcSet as string:

type ImageProps = React.DetailedHTMLProps<
    React.AnchorHTMLAttributes<HTMLImageElement>,
    HTMLImageElement
    > & {
    /**
     * The image field data.
     * @deprecated use field property instead
     */
    media?: ImageField | ImageFieldValue;
    /** Image field data (consistent with other field types) */
    field?: ImageField | ImageFieldValue;
    /**
     * Can be used to explicitly disable inline editing.
     * If true and `media.editable` has a value, then `media.editable` will be processed
     * and rendered as component output. If false, `media.editable` value will be ignored and not rendered.
     */
    editable?: boolean;
    /**
     * Parameters that will be attached to Sitecore media URLs
     */
    imageParams?: {
        [paramName: string]: string | number;
    };
    srcSet?: Array<ImageSizeParameters>;
};

You can check the demo (click at App.tsx) here: https://codesandbox.io/s/xv2wrk3y24

Sitecore / jss